Skip to content
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

Make ticks field protected and updateVelocity utility method public #2139

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

hapily04
Copy link
Contributor

Allows people to more easily create custom movement if necessary for entities, and allows them to update ticks should they not want to call the super tick method.

Copy link
Contributor Author

@hapily04 hapily04 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added javadocs for the updateVelocity method

@hapily04 hapily04 requested a review from txfsjyqm May 15, 2024 18:21
@@ -168,7 +168,7 @@ public void referenceUpdate(@NotNull Point point, @Nullable EntityTracker tracke
private final List<TimedPotion> effects = new CopyOnWriteArrayList<>();

// Tick related
private long ticks;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am definitely opposed to making this field public. It represents ticks alive and should never be updated out of band. If you just want to fetch it, use Entity#getAliveTicks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am definitely opposed to making this field public. It represents ticks alive and should never be updated out of band. If you just want to fetch it, use Entity#getAliveTicks.

The point of this change is to allow subclass members to be able to add to this value when overriding the tick method entirely. If I have a simple NPC class that doesn't do anything except sit still, I don't need the tick method running anything, so at the very least it would be helpful to have the alive ticks be accurate. More importantly, there are some things I just don't need ticking for my arrow implementation, such as effects, as it's a simple arrow implementation, so I want to override the tick entirely and still have the alive ticks be accurate. I'm sure I'm not and won't be the only person who wants to completely override the tick method to remove certain things while still having the alive ticks be accurate.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spoke with @DeidaraMC about this a bit, and we dont agree with each other (he made the same points as you).

My current stance on this is that you should not be encouraged to override the tick method. There are other ways to do remove default entity behavior (hasPhysics = false, override movementTick, etc). Missing cases (like effects) could have a way to disable them, but the single CopyOnWriteArrayList#isEmpty call before effectTick exits is not relevant whatsoever at all. Overriding the tick function completely creates a lot of potential cases where your logic will break because of some unrelated case in minestom where something is handled during tick. This is the reason the update function exists.

If you absolutely must override tick you can create your own field and override getAliveTicks to return your value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spoke with @DeidaraMC about this a bit, and we dont agree with each other (he made the same points as you).

My current stance on this is that you should not be encouraged to override the tick method. There are other ways to do remove default entity behavior (hasPhysics = false, override movementTick, etc). Missing cases (like effects) could have a way to disable them, but the single CopyOnWriteArrayList#isEmpty call before effectTick exits is not relevant whatsoever at all. Overriding the tick function completely creates a lot of potential cases where your logic will break because of some unrelated case in minestom where something is handled during tick. This is the reason the update function exists.

If you absolutely must override tick you can create your own field and override getAliveTicks to return your value.

I suppose it's not entirely important for entity subclasses to have the ability to control the ticks field and I do recognize some importance for keeping at least some base-level consistency between entity classes, but for some thing like the entity synchronization I would need to override the tick method entirely. I could set the system property to something astronomically high, but that's not exactly ideal and would affect any other entity on the server. Also, it shouldn't be encouraged to override the tick method, yes, but it's currently possible, so why not at least encourage people who do override it entirely to keep the ticks field accurate? Here are some ideas I have for possible solution avenues:

  1. Create a shouldSynchronizePosition field to allow people to disable that if needed

  2. Make the ticks field protected

  3. Make everything that currently calls the ticks field directly in the Entity class call getAliveTicks() instead so that we can override that and have everything still work properly if we were to create our own field and update that ourselves.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants