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.

Sign up for GitHub

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

Jump to bottom

Fix not disappearing holograms #4197

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

Fix not disappearing holograms #4197

wants to merge 3 commits into from

Conversation

EpicPlayerA10
Copy link
Contributor

@EpicPlayerA10 EpicPlayerA10 commented May 29, 2024 •
edited
Loading

Description

Fixes not disappearing holograms caused by delayed hologram placement when called from async ( #3176 (comment) )

Proposed changes

  • Makes HologramsService#updateHologram to allows calls only from sync code. Calls from async code should not be allowed as this on its own causes undefined behaviour. For example the HologramsService#removeHologram function already does not allow async.
  • Make some changes in HologramOwner#updateHologram to allow calls from async code. Checks isDeletedSoon to be sure that the hologram owner still exists.

Related Issues (if applicable)

Checklist

  • I have fully tested the proposed changes and promise that they will not break everything into chaos.
  • I have also tested the proposed changes in combination with various popular addons and can confirm my changes do not break them.
  • I have made sure that the proposed changes do not break compatibility across the supported Minecraft versions (1.16.* - 1.20.*).
  • I followed the existing code standards and didn't mess up the formatting.
  • I did my best to add documentation to any public classes or methods I added.
  • I have added Nonnull and Nullable annotations to my methods to indicate their behaviour for null values
  • I added sufficient Unit Tests to cover my code.

@EpicPlayerA10 EpicPlayerA10 requested a review from a team as a code owner May 29, 2024 18:00
@github-actions github-actions bot added the ✨ Fix This Pull Request fixes an issue. label May 29, 2024
@github-actions GitHub Actions
Copy link
Contributor

Your Pull Request was automatically labelled as: "✨ Fix"
Thank you for contributing to this project! ❤️

@JustAHuman-xD
Copy link
Contributor

I’ll try and test and review this today, long standing bug but if it’s being called async that would make sense as to why it does that weird behavior, thanks for the PR!

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented May 29, 2024 •
edited
Loading

Slimefun preview build

A Slimefun preview build is available for testing!
Commit: dd164ef3

https://preview-builds.walshy.dev/download/Slimefun/4197/dd164ef3

Note: This is not a supported build and is only here for the purposes of testing.
Do not run this on a live server and do not report bugs anywhere but this PR!

JustAHuman-xD
JustAHuman-xD requested changes Jun 3, 2024
View reviewed changes
Copy link
Contributor

@JustAHuman-xD JustAHuman-xD left a comment

Choose a reason for hiding this comment

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

Only other thing that may or may not be a problem is the removal of the exception catching and therein logging

src/main/java/io/github/thebusybiscuit/slimefun4/core/services/holograms/HologramsService.java Show resolved Hide resolved
@EpicPlayerA10
Copy link
Contributor Author

EpicPlayerA10 commented Jun 4, 2024 •
edited
Loading

Only other thing that may or may not be a problem is the removal of the exception catching and therein logging

The error will be logged by java anyway because the thrown exception from getHologram will be propagated to the stacktrace. I think it is better to throw exception and stop execution of further code where getHologram was called, than only printing error in the console and allowing further code execution while creating undefined behaviour.

@JustAHuman-xD JustAHuman-xD mentioned this pull request Nov 2, 2024
Draft
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JustAHuman-xD JustAHuman-xD JustAHuman-xD requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
✨ Fix This Pull Request fixes an issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@EpicPlayerA10 @JustAHuman-xD