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

Major Code Rewrite #94

Merged
merged 19 commits into from
Jun 2, 2024
Merged

Major Code Rewrite #94

merged 19 commits into from
Jun 2, 2024

Conversation

OakLoaf
Copy link
Contributor

@OakLoaf OakLoaf commented May 20, 2024

Changes

  • Rewrote HologramData structure
  • Added single schedule thread for holograms
    • This has also been implemented throughout a handful of API methods to ensure correct thread usage
  • Moved EnableChecker class into FancyHologramsPlugin
  • Cleaned up file tree
  • Added more detailed error message when shading API instead of compiling only

I am open to suggestions, and can understand if reverting the api file tree is requested
I have tested this on a local server and have not found any issues, this will of course need testing by others

@@ -101,18 +92,17 @@ public void removeHologram(@NotNull final Hologram hologram) {
Optional<Hologram> optionalHologram = ofNullable(this.holograms.remove(name.toLowerCase(Locale.ROOT)));

optionalHologram.ifPresent(hologram ->
FancyHolograms.get().getScheduler().runTaskAsynchronously(() -> {
final var online = List.copyOf(Bukkit.getOnlinePlayers());
FancyHolograms.get().getHologramThread().submit(() -> {
Copy link

Choose a reason for hiding this comment

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

Execute all the hologram updates in the less executor tasks possible otherwise it will cause notable slowdowns by the massive amount of queued tasks

@bridgelol
Copy link
Contributor

If you want to make this actually decent the whole system should be rewritten with a central "entity tracker", instead of submitting new tasks so often

@OakLoaf
Copy link
Contributor Author

OakLoaf commented May 20, 2024

If you want to make this actually decent the whole system should be rewritten with a central "entity tracker", instead of submitting new tasks so often

I do agree that this would be beneficial, I'd be down to do this. The main goal of this PR initially was just the data rewrite but I thought I might as well try fix the thread issue while I was doing this rewrite. But it definitely can't harm to take this as an opportunity

@OakLoaf
Copy link
Contributor Author

OakLoaf commented May 21, 2024

I have not yet made changes to creating, removing, hiding or showing holograms to players.

Regarding creating and removing I need to get my head around the current implementation before making any changes.

I'll likely make showHologram and hideHologram simply add/remove the player from the viewers list and then let the task in the HologramManager to do the showing and hiding.
This has now been implemented in 07b90f1

@OakLoaf
Copy link
Contributor Author

OakLoaf commented May 21, 2024

I think this pr is relatively read to go now, the only thing I haven't done is changed creating and removing holograms but that could be done in a separate pr (Unless anyone has any immediate improvements that could be made)

@R00tB33rMan
Copy link

Can vouch that this performs exceptionally better than FancyHolograms upstream!

Copy link
Member

@OliverSchlueter OliverSchlueter left a comment

Choose a reason for hiding this comment

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

LGTM

… rewrite

# Conflicts:
#	api/src/main/java/de/oliver/fancyholograms/api/data/DisplayHologramData.java
#	api/src/main/java/de/oliver/fancyholograms/api/data/HologramData.java
#	api/src/main/java/de/oliver/fancyholograms/api/data/TextHologramData.java
#	api/src/main/java/de/oliver/fancyholograms/api/events/HologramUpdateEvent.java
#	api/src/main/java/de/oliver/fancyholograms/api/hologram/Hologram.java
#	src/main/java/de/oliver/fancyholograms/HologramManagerImpl.java
#	src/main/java/de/oliver/fancyholograms/commands/FancyHologramsTestCMD.java
#	src/main/java/de/oliver/fancyholograms/commands/hologram/CreateCMD.java
#	src/main/java/de/oliver/fancyholograms/commands/hologram/VisibilityCMD.java
#	src/main/java/de/oliver/fancyholograms/storage/FlatFileHologramStorage.java
@OakLoaf
Copy link
Contributor Author

OakLoaf commented Jun 2, 2024

This should now have no conflicts, the recent features Visibility, TextColor -> Color and Persistency may need testing though.

@OliverSchlueter OliverSchlueter merged commit da29d13 into FancyMcPlugins:main Jun 2, 2024
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

5 participants