-
Notifications
You must be signed in to change notification settings - Fork 104
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
base: main
Are you sure you want to change the base?
Conversation
if (page.getLines().stream().anyMatch(line -> !line.canShow(player))) { | ||
return; | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 85a2c75
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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".
@@ -571,4 +572,8 @@ public boolean canShow(@NonNull Player player) { | |||
return super.canShow(player) && (parent == null || parent.getParent().canShow(player)); | |||
} | |||
|
|||
// Could be considered hacky but it works? | |||
public boolean shouldAwaitDisplay(Player player) { | |||
return (type == HologramLineType.ICON || type == HologramLineType.HEAD) && player.getTicksLived() < Settings.DEFAULT_MINIMUM_TICKS_LIVED_ITEM_LINE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why you check for head here now? Isn't the issue with floating items alone?
And if it is with heads too, then you have to add small head too...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why you check for head here now? Isn't the issue with floating items alone? And if it is with heads too, then you have to add small head too...
Yeah, appears to be with all items..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Running this on prod atm, works perfectly |
Bumpo, this would be very useful - had a client needing this fixed due to a Fabric Modification that requires similar changes |
Notes: why even bother updating it straight upon join in the first place? We tick every 5 ticks or so, the difference from straight up invoking on join or awaiting ticking wouldn't be noticeable to the client. Not only that, this can cause weird desyncs in the client as well.