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

fix: item lines not showing to players upon joining (#204) #205

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -633,21 +633,21 @@ public boolean show(@NonNull Player player, int pageIndex) {
if (page != null && page.size() > 0 && canShow(player) && isInDisplayRange(player)) {
if (isVisible(player)) {
hide(player);
return true; // Skip a tick before updating, should fix the previous issue with clients desyncing.
}
if (Version.after(8)) {
showPageTo(player, page, pageIndex);
} else {
// We need to run the task later on older versions as, if we don't, it causes issues with some holograms *randomly* becoming invisible.
// I *think* this is from despawning and spawning the entities (with the same ID) in the same tick.
S.sync(() -> showPageTo(player, page, pageIndex), 0L);
}

showPageTo(player, page, pageIndex);
return true;
}
return false;
}
}

private void showPageTo(@NonNull Player player, @NonNull HologramPage page, int pageIndex) {
if (page.getLines().stream().anyMatch(line -> !line.canShow(player))) {
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure this won't cause issues?

If my logic isn't failing me here would this return true if any line of a page can't be shown to a player... Meaning that even if only one of like 10 lines can't be shown, the page wouldn't be shown at all, which could break with existing setups using permissions per line.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed in 85a2c75

Copy link
Contributor

Choose a reason for hiding this comment

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

The main issue remains that if any line returns true, that it skips the whole page that way...

Copy link
Author

Choose a reason for hiding this comment

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

The main issue remains that if any line returns true, that it skips the whole page that way...

It would just send it later?

Copy link
Contributor

Choose a reason for hiding this comment

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

That isn't a good aproach imo, if the entire page is skipped because one line isn't "ready".

Copy link
Author

Choose a reason for hiding this comment

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

That isn't a good aproach imo, if the entire page is skipped because one line isn't "ready".

Well this would require a complete overhaul of the "show" logic, which would also break a good chunk of the API.

Copy link
Author

Choose a reason for hiding this comment

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

That isn't a good aproach imo, if the entire page is skipped because one line isn't "ready".

Could also implement some sort of "queueing" mechanism for the individual line but this is rather dirty imo.

Copy link
Author

Choose a reason for hiding this comment

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

That isn't a good aproach imo, if the entire page is skipped because one line isn't "ready".

edb2adc


page.getLines().forEach(line -> line.show(player));
// Add player to viewers
viewerPages.put(player.getUniqueId(), pageIndex);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -409,6 +409,7 @@ public void show(Player... players) {
if (parent != null && parent.getParent().isHideState(player)) {
continue;
}

if (!isVisible(player) && canShow(player) && isInDisplayRange(player)) {
switch (type) {
case TEXT:
Expand Down Expand Up @@ -568,7 +569,7 @@ public boolean hasFlag(@NonNull EnumFlag flag) {

@Override
public boolean canShow(@NonNull Player player) {
return super.canShow(player) && (parent == null || parent.getParent().canShow(player));
return super.canShow(player) && (parent == null || parent.getParent().canShow(player)) && (type != HologramLineType.ICON || player.getTicksLived() > 40); // Could be considered hacky but it works?
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ public PlayerListener(DecentHolograms decentHolograms) {
@EventHandler(priority = EventPriority.MONITOR)
public void onJoin(PlayerJoinEvent e) {
Player player = e.getPlayer();
S.async(() -> decentHolograms.getHologramManager().updateVisibility(player));
decentHolograms.getPacketListener().hook(player);
if (decentHolograms.isUpdateAvailable() && player.hasPermission("dh.admin")) {
Lang.sendUpdateMessage(player);
Expand Down