-
Notifications
You must be signed in to change notification settings - Fork 2
[bc break!!!] API improvements and documentation improvements #3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
This PR is doing way too many things at once. Please split the PR into individual parts (just like PRs at the PocketMine-MP repo.. it's simply too noisy and prone to errors) |
|
Also, if i read this correctly, this stops custom presets from being used - though i am not 100% sure these worked anyways without modifying the source code of the repo 🤷🏻♂️ |
| rot: new CameraSetInstructionRotation( | ||
| (float)20.0, //pitch | ||
| (float)180.0 //yaw | ||
| (float)20, //pitch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unnecessary change which will make phpstorm and probably phpstan complain (also inconsistent with other occurrences of floating point numbers, see line 84 for example)
| (float) 5.0 // duration (sec) | ||
| ), | ||
| camera_pos: null, | ||
| camera_pos: $player->getPosition()->add(0.0, $player->getEyeHeight(), 0.0), //Without it, the camera will teleport into subspace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i am quite certain this is nullable, it uses the previous/current position
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually test everything
I think it means that once you specify it, it can be null, but in this case, since there is only one specification, it is not possible to specify null
m11.mp4
|
|
||
| After setting the camera to free mode, you need to explicitly assign a target. | ||
| This allows the camera to visually track a specific entity. | ||
| Unlike SetActorLinkPacket, it does not follow the entity automatically. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sentence is simply confusing. It does follow the entity automatically, you just have to instruct it correctly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps I should clarify that it is perspective tracking
|
|
||
| //Manages which packets have been sent | ||
| $this->set[$player->getName()] = true; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not only is this absolute overkill, you also seem to have misunderstood how the instructions send the packets - this, as far as i remember, is automatically handled on send
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would need to unlock it with a sneak click, start it with a normal click, or unlock it over time
| - Multi | ||
|
|
||
| This doesn't work | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could be my fault, some of the recent changes before upload were a bit sus - but we don't have to mention it in the readme. create an issue.
| - registerCustom Preset | ||
|
|
||
| First, build a custom preset registry | ||
| CustomCameraPresetRegistry.php |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be provided by the virion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having an actual implementation of an abstract class reduces the code load
| use pocketmine\scheduler\ClosureTask; | ||
| use pocketmine\entity\Zombie; | ||
| use muqsit\libcamera\CameraPresetRegistry; | ||
| ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this block of use statements? this bloats the README for no real gain. These are also handled by the IDE usually.
| | out_sine | Decelerates toward the endpoint | [see](#out_sine-demo) | ❌ | | ||
| | in_out_sine | Combines both in and out sine behaviors | [see](#in_out_sine-demo) | ❌ | | ||
| | linear | Moves from start to end at constant speed | [see](#linear-demo) | ❌ | | ||
| | spring | Slightly oscillates around the endpoint | [see](#spring-demo) | ✅ | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Easing types are a widely known concept, also they are already mentioned in the bedrock docs (which are linked in the beginning)
This is simply too maintenance heavy to keep in the README, and its overwhelming and unnecessary information for the average user. This readme is about using the virion to enable the cameras, not to explain the whole system itself. Those things should be handled by wikis if anything.
| if(!$player->isSneaking()){ | ||
| return; | ||
| } | ||
| if($player instanceof Player&&$player->isOnline()){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
formatting - or just yeet it all
|
This is just way too much useless bloat and information in the README. I don't have the time, will or energy to review these unnecessary changes. Debloat, then request another review please. |
|
This is a duct-tape API improvement created for use on my server(ver 1.21.93), so I no longer intend to have it merged, but I will respond to this review to some extent |
new APIs
We no longer have to type
libcamera::getPresetRegistry()->registeredevery timeWith the new API it looks like this:
preset: CameraPresetRegistry::FREE(),Want to use a preset from a string? Use
match()Want to use a preset with partial matches? Use
str_start_with()The following functions have been added:
The following functions have been removed:
libcamera::getPresetRegistry()New exceptions added
Improved readme
Added documentation with various tested use cases and videos
Linked Issues
closed #2
About Squash Merge
I am committing with the assumption that it will be squash-merged, so please squash-merge.