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

Add advancement progress tracker (#4364) #4568

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

Conversation

eclipseisoffline
Copy link
Contributor

@eclipseisoffline eclipseisoffline commented Apr 14, 2024

This pull request adds a progress tracker to the Bedrock advancements menu opened by /geyser advancements (#4364), and additionally fixes a problem I have noticed that causes the advancements menu to break in rare scenarios.

A few notes:

  • The AdvancementsCache#isEarned method now internally uses the new AdvancementsCache#getProgress method, to prevent code duplication.
    • This has a slight impact on performance, since AdvancementsCache#isEarned used to return as soon as it had discovered one requirement in the progress tracker that hasn't been met.
  • The AdvancementsCache#buildAndShowInfoForm negates this performance impact a bit by only calling the getProgress method once, instead of calling both isEarned and getProgress.
  • To show the progress text a new language key has been added. Pull request #126.
  • Lastly, when using the Geyser advancements menu with datapacks/mods that add advancements I have noticed a few rare scenarios where the advancements menu fails with a NullPointerException. I fixed this by checking for non-existent parents in GeyserAdvancement.

@onebeastchris
Copy link
Member

Couldn't the progress language key be used from the Java translations?
Geyser downloads those for each locale

@onebeastchris onebeastchris added the PR: Feature When a PR implements a new feature label Apr 14, 2024
@eclipseisoffline
Copy link
Contributor Author

eclipseisoffline commented Apr 14, 2024

Java doesn't display a Progress: text in the advancement menu. I have looked through its translation keys and it seems there is no similar key available. There is a key available for the progress counter itself (advancements.progress, which is %s/%s for American English), which I'll implement shortly.

2024-04-14_12 11 30

Copy link
Member

@onebeastchris onebeastchris left a comment

Choose a reason for hiding this comment

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

Thanks! This looks good to me, will do some testing tomorrow.
Unfortunately, this PR won't be merged immediately as it adds a language string - currently, the cloud PR has to come first before language strings can be updated (could be a few days, it's nearing completion now)

@onebeastchris onebeastchris added the PR: On hold When a PR is on hold like if it requires a dependency to be updated first label Apr 16, 2024
@rtm516
Copy link
Member

rtm516 commented Apr 21, 2024

Can we get before/after screenshots?
Also has there been any reported issues around the NPE reported we can close with this?

@eclipseisoffline
Copy link
Contributor Author

I've attached before/after screenshots. As you'll see it's a very slight change that only appears when viewing a specific advancement.

I didn't find any issues reported relating to the NPE, and since the fix was small I decided not to report one and just include it in this PR. If you're interested, here's the exception that would appear in the logs and prevent the advancements menu from opening:

[10:15:00] [Geyser player thread-5-1/ERROR]: Could not translate packet ClientboundSelectAdvancementsTabPacket
java.lang.NullPointerException: Cannot invoke "org.geysermc.geyser.level.GeyserAdvancement.getParentId()" because "advancement" is null
	at org.geysermc.geyser.level.GeyserAdvancement.getRootId(GeyserAdvancement.java:86) ~[Geyser-Fabric.jar:?]
	at org.geysermc.geyser.session.cache.AdvancementsCache.buildAndShowListForm(AdvancementsCache.java:126) ~[Geyser-Fabric.jar:?]
	at org.geysermc.geyser.translator.protocol.java.JavaSelectAdvancementsTabTranslator.translate(JavaSelectAdvancementsTabTranslator.java:44) ~[Geyser-Fabric.jar:?]
	at org.geysermc.geyser.translator.protocol.java.JavaSelectAdvancementsTabTranslator.translate(JavaSelectAdvancementsTabTranslator.java:37) ~[Geyser-Fabric.jar:?]
	at org.geysermc.geyser.registry.PacketTranslatorRegistry.translate0(PacketTranslatorRegistry.java:89) ~[Geyser-Fabric.jar:?]
	at org.geysermc.geyser.registry.PacketTranslatorRegistry.lambda$translate$0(PacketTranslatorRegistry.java:69) ~[Geyser-Fabric.jar:?]
	at io.netty.util.concurrent.AbstractEventExecutor.runTask(AbstractEventExecutor.java:174) ~[netty-common-4.1.97.Final.jar:?]
	at io.netty.channel.DefaultEventLoop.run(DefaultEventLoop.java:54) ~[netty-transport-4.1.97.Final.jar:?]
	at io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:997) ~[netty-common-4.1.97.Final.jar:?]
	at io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74) ~[netty-common-4.1.97.Final.jar:?]
	at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30) ~[netty-common-4.1.97.Final.jar:?]
	at java.lang.Thread.run(Thread.java:833) ~[?:?]

Before:
Before

After:
Screenshot_20240421_191352

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Feature When a PR implements a new feature PR: On hold When a PR is on hold like if it requires a dependency to be updated first
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants