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

Fixed TextDisplay's offsets. #4676

Merged
merged 12 commits into from
Jul 7, 2024
Merged

Conversation

stromsoftware
Copy link
Contributor

Fixed TextDisplay's offsets by adding the Translation (https://wiki.vg/Entity_metadata#Entity_Metadata_Format), mainly for passenger entities.

@stromsoftware stromsoftware requested a review from rtm516 May 19, 2024 15:51
@stromsoftware
Copy link
Contributor Author

Why is this still not approved?

Copy link
Member

@onebeastchris onebeastchris left a comment

Choose a reason for hiding this comment

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

Here you go - after these, this should be good to merge. If you have questions about how to implement specifics, feel free to reach out via discord.

@onebeastchris
Copy link
Member

Looks better now, but if you're alright with that, I'd push a few minor changes to this PR myself - I'm pretty sure that we could cancel the entity mount offset from getting applied in a way that doesn't involve adding an extra check to that static method.

@stromsoftware
Copy link
Contributor Author

stromsoftware commented Jun 19, 2024

Looks better now, but if you're alright with that, I'd push a few minor changes to this PR myself - I'm pretty sure that we could cancel the entity mount offset from getting applied in a way that doesn't involve adding an extra check to that static method.

It was messing with the offsets until I added that check, it seems like the function is triggering before setTranslation is being fed which results in the null value on baseTranslation and doesn't set offsets. But the tricky part here is that it doesn't happen every time and not always on every passenger, so the issue isn't that easy to fix and will likely require lot of testing or you could mess it up quite easily.

If you can't fix it quickly it's best to leave the safety check, this check is not being called that often to mess with performance even at 10k TextDisplay passengers (tested).

Copy link
Member

@onebeastchris onebeastchris left a comment

Choose a reason for hiding this comment

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

I've pushed some smaller cleanup changes (codestyle fixes, removed the hasTranslation variable) - just one final thing, and then this should be good to be merged. Sorry for the wait!

@onebeastchris onebeastchris added the PR: Bugfix When a PR contains a bugfix label Jul 3, 2024
Copy link
Member

@onebeastchris onebeastchris left a comment

Choose a reason for hiding this comment

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

Looks okay now. Thanks!

@onebeastchris onebeastchris merged commit e0af0a5 into GeyserMC:master Jul 7, 2024
2 checks passed
OurLobanov added a commit to fmine-be/Geyser that referenced this pull request Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Bugfix When a PR contains a bugfix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants