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

Conversation

bridgelol
Copy link

@bridgelol bridgelol commented Apr 17, 2024

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.

Comment on lines 647 to 649
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

@bridgelol bridgelol changed the title fix: item lines not showing to players upon joining (#204) WIP: fix: item lines not showing to players upon joining (#204) Apr 17, 2024
@@ -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;
Copy link
Contributor

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...

Copy link
Author

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..

Copy link
Author

Choose a reason for hiding this comment

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

@bridgelol bridgelol changed the title WIP: fix: item lines not showing to players upon joining (#204) fix: item lines not showing to players upon joining (#204) Apr 17, 2024
@bridgelol
Copy link
Author

Running this on prod atm, works perfectly

@JoshTQ
Copy link

JoshTQ commented Aug 2, 2024

Bumpo, this would be very useful - had a client needing this fixed due to a Fabric Modification that requires similar changes

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.

3 participants