-
Notifications
You must be signed in to change notification settings - Fork 20
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
HUD Text API #122
base: develop
Are you sure you want to change the base?
HUD Text API #122
Conversation
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.
Mostly surface-level concerns. The implementation is otherwise very good, and I'm more than happy to merge this once my issues are addressed.
src/test/java/net/modificationstation/sltest/gui/hud/TestHudText.java
Outdated
Show resolved
Hide resolved
...in/java/net/modificationstation/stationapi/impl/client/gui/hud/StationFlatteningHudText.java
Outdated
Show resolved
Hide resolved
.../main/java/net/modificationstation/stationapi/impl/client/gui/hud/StationVanillaHudText.java
Outdated
Show resolved
Hide resolved
...main/java/net/modificationstation/stationapi/impl/client/gui/hud/StationRendererHudText.java
Outdated
Show resolved
Hide resolved
src/test/java/net/modificationstation/sltest/gui/hud/TestHudText.java
Outdated
Show resolved
Hide resolved
...0/src/main/java/net/modificationstation/stationapi/api/client/event/gui/hud/HudTextLine.java
Outdated
Show resolved
Hide resolved
...ain/java/net/modificationstation/stationapi/api/client/event/gui/hud/HudTextRenderEvent.java
Show resolved
Hide resolved
.../main/java/net/modificationstation/stationapi/impl/client/gui/hud/StationVanillaHudText.java
Show resolved
Hide resolved
...ain/java/net/modificationstation/stationapi/api/client/event/gui/hud/HudTextRenderEvent.java
Show resolved
Hide resolved
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.
Last concern.
...0/src/main/java/net/modificationstation/stationapi/api/client/event/gui/hud/HudTextLine.java
Show resolved
Hide resolved
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.
done
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.
Surface level review, but I have some big concerns:
HudTextLine
being instantiated dozens of times each frame.- Vanilla debug renderer being completely skipped.
In my opinion, HudTextLine should instead contain a template string and an array of callbacks for formatting, so that it can be instantiated once and always be up-to-date.
For example: Template "Used memory: %d\% (%dMB) of %dMB"
, the array of callbacks being:
var callbacks = new Supplier[] {
() -> (Runtime.getRuntime().totalMemory() - Runtime.getRuntime().freeMemory()) * 100L / Runtime.getRuntime().maxMemory(),
() -> (Runtime.getRuntime().totalMemory() - Runtime.getRuntime().freeMemory()) / 1024L / 1024L,
() -> Runtime.getRuntime().maxMemory() / 1024L / 1024L
}
This is just a working example, this could be optimized by using a single class with its methods registered as callbacks, so the calculated values could be shared.
Making HudTextLine use callbacks also enables the debug HUD to just be built once instead of each frame.
As for the vanilla lines being completely canceled, I feel like a more surgical approach must be taken where there's a way to cancel each line (maybe by using something like EnumSet<VanillaHudLine> canceled
?) and then overwrite the freed space while building the debug HUD. StAPI shouldn't have to render Minecraft's debug if it's unchanged.
My own two cents, StAPI replacing the vanilla debug hud is perfectly fine, IMO. It keeps things simple on the impl side and overall makes it easier to maintain. Concerns of other mods affecting the vanilla hud are honestly a nil point to me due to fact we can't predict what lines will be shifted around, and to where. I'm also not too concerned about the object churn, but it would be nice to have an event+registry for hud lines so modders can cancel specific lines without mixins. |
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.
Wrong PR lmao, I should look at my tabs.
A simple API for adding (debug) text to the game HUD.