Add Hud Render Events

[kevinthegreat1]

Originally discussed here.

Deprecate HudRenderCallback.

Fixes #3908.

[Fuzss]

Not sure this really goes far enough. The system used by NeoForge has always been pretty ideal (although quite invasive): splitting gui rendering into all the different parts (health, hotbar, status effects, etc.) and allowing mods to render their custom gui layers anywhere in-between.

Also it would be awesome to finally get shared height parameters for bars (health bar + armor bar on the left side, food + air on the right side) rendered above the hotbar in Fabric Api. So that multiple mods adding their own bars know at what height to render without interfering with others.

[kevinthegreat1]

Not sure this really goes far enough. The system used by NeoForge has always been pretty ideal (although quite invasive): splitting gui rendering into all the different parts (health, hotbar, status effects, etc.) and allowing mods to render their custom gui layers anywhere in-between.

I agree that Forge's implementation (last when I saw it) is much more versatile and comprehensive. It would be great to be able to cancel certain components too. However as you mentioned it is quite invasive, and in the past, this injection point hasn't exactly been the most stable. A more comprehensive api would need a bit more discussion with the maintainers, but I'd be happy to see it.

Also it would be awesome to finally get shared height parameters for bars (health bar + armor bar on the left side, food + air on the right side) rendered above the hotbar in Fabric Api. So that multiple mods adding their own bars know at what height to render without interfering with others.

The height problem is just reading the code to see the height where bars and components render right? Are you suggesting we provide z constants for different HUD elements?

[Fuzss]

The height problem is just reading the code to see the height where bars and components render right? Are you suggesting we provide z constants for different HUD elements?

No. NeoForge adds Gui#leftHeight and Gui#rightHeight. Those update automatically as vanilla bars are drawn above the hotbar. Mods drawing their own bars should update them as well. So e.g. if I have a mod that draws an armor toughness bar above the vanilla armor bar on the left hotbar side I know at what height to draw my bar at, and then another mod that also draws a bar at the left hotbar side can use the left height variable that has been updated by me to draw above me once more.

[modmuss50]

Im not sure events like this are the best way to do this. An API that allows you to register LayeredDrawer.Layer's in the desired order may be better?

[modmuss50]

Are events like this the best solution here? Would it be better to allow mods to register their own LayeredDrawer.Layer in a similar way to how the vanilla hud elements work?

[kevinthegreat1]

I have updated the code to use one interface, but I don't see a difference with registration methods. Wouldn't it still be implemented with an event?

[modmuss50]

I was thinking they could just be static? I dont think there is a need for an even unless im missing something.

[kevinthegreat1]

I was under the impression that not using events is more complicated than just using events? So there's no need for registration methods? Are you saying just implement it with a synchronized list? Events also allow phase ordering if some mod wants to render before another.

[Octol1ttle]

Yes, as discussed on discord I think it would be better to statically register (or maybe register in events similar to the ones in the current state of the PR a vanilla LayeredDrawer.Layer.

Done in 0d2bcf2.

I think what modmuss meant here is that we would have a HudRenderRegistry (or HudRenderRegistrationEvents) where mods would put their LayeredDrawer.Layers. Then, all these layers would be added to the drawer directly with addLayer

[kevinthegreat1]

I think what modmuss meant here is that we would have a HudRenderRegistry (or HudRenderRegistrationEvents) where mods would put their LayeredDrawer.Layers. Then, all these layers would be added to the drawer directly with addLayer

He said it’s fine to register in events. I used events because it allows ordering the layers within one phase/event.

Also, directly adding to the vanilla list would require similarly many injection points or rely on the indices which is even more undesirable.

[maityyy]

Maybe END? There is START, not FIRST or smth

[kevinthegreat1]

The names are taken from WorldRenderEvents, which has both LAST and END, and LAST is the last place where you should render stuff in most cases. However, I'm happy to change this if modmuss agrees.

[modmuss50]

This is looking really good now, I have just a few comments (mostly about my own changes :D ) Do let me know if you have any questions or disagree.

[modmuss50]

Maybe remove hud from the names? Might be worth looking over these names in general to make sure they are consitent.

[kevinthegreat1]

I've tried to update the names to be consistent.

[modmuss50]

I wonder what automated testing we can do with the new client tests? Might need to wait until #4381 has been merged. Don't let this block this PR, but its a nice thing to keep in mind.

[kevinthegreat1]

Yep, I was planning for that. All the tests in here can be easily made automated after the screenshots stuff become available.

[PepperCode1]

Currently, adding a layer relative to a layer in a SubLayer will also apply the SubLayer's predicate to the new layer, which should not happen. This should be solved by flattening the SubLayer when such an addition is requested, so any new layers are always added directly to the layer list of LayeredDrawerWrapperImpl.base while preserving layer order.

[kevinthegreat1]

I think it makes sense to have the default to respect hide hud, but I also see the need to register layers that always render. I tried to make a solution that does both, but I was only able to come up with a working solution when sub-layers are only one level deep (sub-layers don't contain sub-layers).

Basically I added a ignoreShouldRender method to our layer interface. In SubLayer, if the sub-layer shouldn't be rendered, we manually render all layers that returns true for ignoreShouldRender. This, however, does not work with nested sub-layers.

[PepperCode1]

I don't agree that it makes sense for the default to respect the existing predicate, and I think adding additional methods only adds unnecessary complexity for the user when they can very easily add their own check for whether the HUD is hidden. The existing predicate also doesn't even exist all the time, nor do I think the API should need to expose exactly how layers are bundled internally.

[PepperCode1]

To further illustrate my point, if I call wrapper.addLayerBefore(SLEEP, myLayer), it will have no additional predicate besides what my layer checks because the SLEEP layer is not in a SubLayer. However, if I call wrapper.addLayerAfter(BOSS_BAR, myLayer), it will now have the additional "HUD hidden" predicate because the BOSS_BAR layer is in a SubLayer with that predicate. This is not what a user would expect because BOSS_BAR and SLEEP are right next to each other in the vanilla layer order. This behavior is not even documented currently, but I am arguing that even if it was, that would be adding a lot of documentation (effectively "adding after, adding before, or replacing this layer will apply an additional predicate to the new layer" or "adding after, adding before, or replacing this layer will not apply an additional predicate to the new layer" for each layer in IdentifiedLayer) for this inconsistent behavior. Additionally, a method would have to be added to IdentifiedLayer (the proposed ignoreShouldRender) for whether to ignore an additional predicate, with up to 4 more overloads in LayeredDrawerWrapper to make it convenient to use. I do not understand the benefit of adding all of this complexity to the API when the alternative does not require any of it. The only benefit I see is that the user will sometimes not have to write if (MinecraftClient.getInstance().options.hudHidden) { return; } inside their layer.

[kevinthegreat1]

I agree the behavior can be confusing. I am always happy to add documentation, and the current documentation is in no way final. However, one of the original goals of this pr and the original discussion is that hide hud is automatically applied, as HudRenderCallback originally did before it got broken, so I still believe that hide hud and predicate should be respected, at least an option to be respected.

[Octol1ttle]

Maybe we can change the terminology: instead of adding a layer before/after a vanilla layer, one "attaches" their layer to the vanilla layer.

I also stand by having the original predicate respected in modded layers.

[modmuss50]

This is a valid point that @PepperCode1 raises, I think there are maybe 3 soltuions:

1. Flattern the layers, this was something we were trying so hard not to do.
2. As @Octol1ttle suggested changing the naming/documentation to make it clear how it currently works.
3. Add a layer twice, once after the target layer and once after the sub layer, drawing only one at a time.

Im leaning towards saying option 2 is the way to go, we could maybe rename the layers hidden by this flag to BOSS_BAR_HUD for example. I dont think there is an ideal solution, we just need to pick the compromise we want to take.