Port to fabric 1.15.2, 2nd pass

[Sturmlilie]

Ready to merge

Changes since first pass:

• Remove unrelated gradle-wrapper upgrade
• Undo version change
• DiffuseLighting.enableForItems() -> DiffuseLighting.enableGuiDepthLighting()
• Move infrared renderer init to client bootstrap code
• Reworde big comment in RenderUtil about white dummy texture
• Reinstate LeakDetector in DisplayModule
• Remove XXX comment in AudioModule about rendered appearance
• Undo Queue/Stack merger
• Remove XXX since ActionResult.SUCCESS is correct for hand motion when right clicking casing block
• Improve SequencerModule rendering port by relinquishing transparent quads
• Add @Environment annotations whereever possible
• Add javadoc for most things
• Squash / split a bunch of followup commits (general cleanups)
• Test everything on dedicated server

I'm kinda displeased that github doesn't show the commit order properly. :sweat:

[Sturmlilie]

Fixed missing javadoc for one of the RenderUtil funcs

[Sturmlilie]

Noticed an unfortunate bug in the depth-based stenciling of the manual GUI text, it also affects tooltips for some reason:

Seems minor enough to fix once the 1.15 port lands though.

[Sturmlilie]

Turns out it was as easy as clearing the depth buffer after the document contents were rendered. If need to make a 3rd PR iteration I'll include it there, otherwise I'll PR it separately.

[Sturmlilie]

[fnuecke]

Picking up the conversation from the previous PR here so I don't overlook replies in the future:

Queue/Stack

Haha, ok cool :) Honestly, sleeping over it (a few times) I like it. I'm pretty sure my iteration concerns are unfounded, seeing how BlockPos gets spammed all over the place in MC already, so a few more closures probably won't even register. So it's definitely worth the reduced redundancy. My only other concern was that my first impression was readability was a bit worse this way, but now I'm pretty sure that was 99% bias of me knowing the old code but not the new code, heh.

UI

Ah, that makes sense I guess. If you're up to it, another PR some day to future-proof the UI code would be very much appreciated. But I don't think there's any short-term payoff in that, so unless you're really really bored, I'd say don't bother until 1.16 or so comes around, when it becomes necessary/has stabilized in MC/makes more sense.

Dedicated Server

Haha, don't worry about it. It took me around probably three or more published mod builds with people yelling at me until I properly internalized the necessity of testing the dedicated server separately :D

Font rendering

Hmm, right. To not unnecessarily break compatibility, probably new overloads taking the type? So in api.detail.FontRendererAPI something like this maybe?

    enum FontRendererType {
        SMALL,
        NORMAL,
    }
    void drawString(FontRendererType type, final String string);
    void drawString(FontRendererType type, final String string, final int maxChars);
    int getCharWidth(FontRendererType type);
    int getCharHeight(FontRendererType type);

Unless you have a better idea? One alternative could be to have a second FontRendererAPI field in API, but then that would have to be used directly, which doesn't really fit in with the rest of the API, so I don't really like it.

Manual depth issue

If it's an easy fix, it'd be cool to get it in with the PR. I'll create a 1.15 branch in the meantime so you can create the PR on that. Edit: https://github.com/MightyPirates/TIS-3D/tree/master-MC1.15-fabric

Screenshot

Hmm?

[Sturmlilie]

Picking up the conversation from the previous PR here so I don't overlook replies in the future:

Queue/Stack

Haha, ok cool :) Honestly, sleeping over it (a few times) I like it. I'm pretty sure my iteration concerns are unfounded, seeing how BlockPos gets spammed all over the place in MC already, so a few more closures probably won't even register. So it's definitely worth the reduced redundancy. My only other concern was that my first impression was readability was a bit worse this way, but now I'm pretty sure that was 99% bias of me knowing the old code but not the new code, heh.

The reason I attempted it in the first place was because I saw it already being done with the RAM / ROM modules. However you were right that such a refactor is not strictly part of the porting effort; if you still want it in, I can either make a followup PR that is just pure refactoring, or bring back the commits from the first PR pass.

UI

Ah, that makes sense I guess. If you're up to it, another PR some day to future-proof the UI code would be very much appreciated. But I don't think there's any short-term payoff in that, so unless you're really really bored, I'd say don't bother until 1.16 or so comes around, when it becomes necessary/has stabilized in MC/makes more sense.

It might come sooner than later (see below).

Font rendering

Hmm, right. To not unnecessarily break compatibility, probably new overloads taking the type? So in api.detail.FontRendererAPI something like this maybe?

    enum FontRendererType {
        SMALL,
        NORMAL,
    }
    void drawString(FontRendererType type, final String string);
    void drawString(FontRendererType type, final String string, final int maxChars);
    int getCharWidth(FontRendererType type);
    int getCharHeight(FontRendererType type);

I was about to suggest a provider method returning a reference to detail.FontRendererAPI per FontRendererType, but then I realized all the outwards facings methods in the top-level api package are static, so your suggestion fits better.

Unless you have a better idea? One alternative could be to have a second FontRendererAPI field in API, but then that would have to be used directly, which doesn't really fit in with the rest of the API, so I don't really like it.

Manual depth issue

If it's an easy fix, it'd be cool to get it in with the PR. I'll create a 1.15 branch in the meantime so you can create the PR on that. Edit: https://github.com/MightyPirates/TIS-3D/tree/master-MC1.15-fabric

Yeah it's literally just a one-liner, I'll push it up. Thanks for the branch.

Screenshot

Hmm?

Working on 1.16, prereleases are already dropping since yesterday or so, therefore I have a somewhat stable target to port towards =)
TIS-3D wise, the main casualties are the GUI classes; I haven't gauged yet if the changes needed are minor or if the rendering related refactors might have to be done sooner than expected. So far I'm just done stubbing most things out.

[fnuecke]

Queue/Stack

Ahh, my bad, hadn't looked through the full changeset of this PR again. OK, that's cool then. Feel free to follow that up with its own PR :)

1.16

Ohh, very nice!

[Sturmlilie]

Come to think of it, I should probably make a minimal extension mod implementing a module, just to ensure that the outward-facing API is functional.

[Sturmlilie]

Oh by the way, how are you planning on making your fixes for #102 and #103 propagate up into the higher branches?

[fnuecke]

OK, so that took longer than it had to because I didn't realize I still had an outdated version of the 1.14 branch locally. Ugh.

I forgot how aggressively I renamed things, so I'm pulling the patches through via (fake) cherry-picking.

Note that I recreated the 1.15 branch based on the previous 1.14 branch as it was here, so ignore the new conflicts here. When PR-ing against the 1.15 one there shouldn't be any. The conflicts here are 99% mapping stuff, since I had to update Fabric and mappings to get 1.14 build locally again for some reason... so I'll just (fake) cherry-pick those two fixes over after the PR.

Sorry for that extra confusion...

[Sturmlilie]

Sorry for that extra confusion...

Don't worry about it, I understand what happened =)
Thanks for leaving the 1.15 branch pointing at the old 1.14; I wouldn't have minded resolving the conflicts but the cherry-picking method is less time consuming, so why not.
Edit: I'm leaving this PR pointed at the wrong branch for now until I've pushed the Font API fixes, so you don't accidentally merge before that ;)

[Sturmlilie]

Okay, so an update on the (Font) API situation: The helpful people on fabricord have explained to me how mod initialization ordering would work in fabric; in short, it's similar to how mods themselves are initialized. From TIS-3D's point of view, we would have an ExtensionInitializer interface in the api package (not unlike ModInitializer) which extensions would implement and register in their mod.json, and then during our own init, we could query all implementations of ExtensionInitializer from the loader and invoke them, guaranteeing that our own API classes have been setup.
Correct me if I'm wrong, but the reason all entrypoints in TIS-3D's API are static and null-checked is because in previous versions, there was no guarantee about init-phase ordering of mods?
Anyway, with doing it the proper fabric way I could get away from that. But the main point I'm trying to make is that currently it makes little sense to touch the APIs since they don't work the proper fabric way anyway, and I'd suggest implementing this in a separate PR so it doesn't bog this one down even further. What are your thoughts?

[Sturmlilie]

Mockup of what I described earlier:
Base mod changes: https://github.com/Sturmlilie/TIS-3D/compare/1.15-fabric-port-v2...Sturmlilie:api-fabric-port
Sample extension skeleton: https://github.com/Sturmlilie/TIS-3D-Additions

[fnuecke]

Mod extension stuff: interesting! Yes, the main reason those are public static is because there really was nothing like dependency injection or such in the past. So that's been the most decoupled "clean" way. Plus mods liked to bundle other mods' APIs back in the day (before Forge's Optional annotations) so the null-checks avoided breakage in that case. And it was also the most convenient to just be able to use it from wherever. There's probably been ways to do that differently even in Forge nowadays.

So the main gain would be to get rid of the statics and null checks, yes? And I guess the most basic conversion would be having a 'main' API interface with getters for what are now fields in the API class [1], an implementation of which gets passed to [Client]ExtensionInitializer.onInitialize? And then it's the mods' responsibility to keep a reference of that around for future communication?

What's the initialization order here then; would mods have to define themselves as being a dependency of TIS-3D for them to be initialized before the extension initialization code gets run? Would Fabric do that automatically because it knows what other mod the interface they're implement is coming from? Would Fabric run the initialization of the mods that declare an implementation of the extension interfaces at the time those are retrieved if that hasn't happened yet? Something else?

Separate PR for that sounds good :)

(BTW, since you're on a roll, if you want to look into making a PR porting the Facade Module [2] from 1.12 to 1.15+... that'd be cool! :D)

[1] I feel splitting it up into an initializer per sub-api could be annoying when you need more than one? More flexible in the long run, of course, but not sure that's needed here. [2] Part 1, 2 and 3

[Sturmlilie]

Mod extension stuff: interesting! Yes, the main reason those are public static is because there really was nothing like dependency injection or such in the past. So that's been the most decoupled "clean" way. Plus mods liked to bundle other mods' APIs back in the day (before Forge's Optional annotations) so the null-checks avoided breakage in that case. And it was also the most convenient to just be able to use it from wherever. There's probably been ways to do that differently even in Forge nowadays.

I see, that explains a lot.

So the main gain would be to get rid of the statics and null checks, yes? And I guess the most basic conversion would be having a 'main' API interface with getters for what are now fields in the API class [1], an implementation of which gets passed to [Client]ExtensionInitializer.onInitialize? And then it's the mods' responsibility to keep a reference of that around for future communication?

What's the initialization order here then; would mods have to define themselves as being a dependency of TIS-3D for them to be initialized before the extension initialization code gets run? Would Fabric do that automatically because it knows what other mod the interface they're implement is coming from? Would Fabric run the initialization of the mods that declare an implementation of the extension interfaces at the time those are retrieved if that hasn't happened yet? Something else?

Separate PR for that sounds good :)

Great! I'd answer your post in detail but I feel that discussion should live in the separate PR.
So from my side, this one can be merged then =) I've adjusted the target branch to 1.15 accordingly.

(BTW, since you're on a roll, if you want to look into making a PR porting the Facade Module [2] from 1.12 to 1.15+... that'd be cool! :D)

Oh yeah, actually a short while ago I got curious why 1.12 was ahead of me by a few commits, and I stumbled upon it. Definitely want to make it happen, but I feel I'll have to dive deeper into Minecraft and Fabric for that, so it's further back on my list (having this mod available for 1.16 at launch is my current goal).

[1] I feel splitting it up into an initializer per sub-api could be annoying when you need more than one? More flexible in the long run, of course, but not sure that's needed here. [2] Part 1, 2 and 3

[fnuecke]

Great, thanks again!