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

Adjust when the EntityTrackingEvents are fired. #4369

Merged
merged 3 commits into from
Jan 14, 2025

Conversation

Octol1ttle
Copy link
Contributor

@Octol1ttle Octol1ttle commented Jan 9, 2025

The current implementation of the two events in EntityTrackingEvents isn't particularly useful. Both events fire at a bad timing - the client no longer knows of the entity by the time the callback is triggered. Attachment Sync had to implement its own mixin to work around this.

This PR changes so that START_TRACKING fires after the initial entity packets have been sent to the client and STOP_TRACKING fires before the destroy packets are sent.

These changes result in mods being able to have infinitely more functionality associated with these events, because the client is aware of the entity's existence in both events (e.g. StartTracking can finally be used in Attachment Sync)

Supersedes #3696. I don't think having extra events is the solution to this problem.

@Octol1ttle Octol1ttle changed the title Fix EntityTrackingEvents at once Fix EntityTrackingEvents Jan 9, 2025
Copy link
Contributor

@Syst3ms Syst3ms left a comment

Choose a reason for hiding this comment

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

Possibly a breaking change behavior-wise, but I would be surprised if any mods actually relied on the old behavior's quirkiness. Worth looking out for, though.

@Linguardium
Copy link

in my opinion, to prevent a breaking change, a new event could be added in the desired places. maybe add a before and after event for both, deprecate the existing event and call it from the before event. that way its clear when the events are called, as well

@Octol1ttle
Copy link
Contributor Author

This "breaking change"... is it in the room with us right now?

I've yet to see any real-world case where existing users of these events would encounter non-trivial problems with this PR's implementation. When I used GitHub's global search to find usages of these events, a significant majority of them was S2C communication, which only benefits from this PR

As mentioned in the PR description, I don't want "before" and "after" events because half of them will only be used in <1% of consumer code.

Copy link
Member

@modmuss50 modmuss50 left a comment

Choose a reason for hiding this comment

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

Im happy with this, it makes much more sense.

@modmuss50 modmuss50 added enhancement New feature or request merge me please Pull requests that are ready to merge labels Jan 14, 2025
@modmuss50 modmuss50 changed the title Fix EntityTrackingEvents Adjust when the EntityTrackingEvents are fired. Jan 14, 2025
@modmuss50 modmuss50 merged commit 8998135 into FabricMC:1.21.4 Jan 14, 2025
4 checks passed
@Octol1ttle Octol1ttle deleted the fix-starttracking-at-once branch January 14, 2025 13:48
modmuss50 pushed a commit that referenced this pull request Jan 14, 2025
* fix EntityTrackingEvents at once

* use event in Attachment Sync API

* fix: remove removed mixin from mixins.json

(cherry picked from commit 8998135)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request merge me please Pull requests that are ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants