MightyPirates / TIS-3D

TIS-100 inspired low-tech computing in Minecraft.
https://www.curseforge.com/minecraft/mc-mods/tis-3d
Other
108 stars 35 forks source link

Port to fabric 1.15.2, initial feedback pass #100

Closed Sturmlilie closed 4 years ago

Sturmlilie commented 4 years ago

Do not merge, this PR is against the 1.14 branch!

Hi! I took it upon myself to port the fabric branch to 1.15.2. This is my first time doing anything with MC modding / fabric / Java. The branch is not yet in a state to be merged, but it's very playable at this point and so I wanted to get an initial round of feedback before I progress. The biggest changes in 1.15 were rendering related, and so this is also what most of my code touches. I've made incremental changes in order for every commit (except the first few ones) to build and load, so if I replaced a certain interface, often I left the old methods in tact for the code to still build (eg: RenderUtil:drawQuad); eventually those old interfaces will be removed when their last user is gone / converted.

At this point every module renders (albeit with some caveats), and the core game mechanics are intact. There were a couple apparent behavior bugs (~like copying ROM from RAM modules~) that I haven't touched yet because they also appear in the 1.14 branch this is based on.

Things that are explicitly broken by me:

Regarding module overlay drawing

In the past this was done via immediate OpenGL and so the programmer had maximum flexibility. With the new render pipeline, submitted quads are not rendered on the spot and their ordering is also not defined, so instead of the painter's algorithm we have to either not overlap quads or rely on the depth buffer. I was able to mostly work around this by only drawing opaque quads and slightly bumping certain ones within the 0.005 Z-axis margin from the casing block (see CasingBlockEntityRenderer).
Now, if one wanted to keep that flexibility (which I wouldn't advocate for), all of the module overlay rendering would have to be moved to a dynamic texture, ie. every module get's their own X by X space on an FBO and draws their state in whatever fashion onto it, and the casing renderer then uses that to texture itself.
However, seeing how I was able to reproduce 90% of the modules with the existing approach, I don't think it's worth the hassle.

Note that most of the methods added by me lack javadoc comments; if there are no bigger rewrites necessary, I will comment as many as possible in a followup commit.

Please feel free to critique any part of my code, I'm trying to learn =)

fnuecke commented 4 years ago

This is awesome! Thanks so much, I really appreciate it :)

I'll browse through the changes, but don't expect me to be able to tell you anything when it comes to 1.15, I expect I'll learn a lot from the diffs myself.

I did give it a quick start, and looks good indeed!

Responding to the dynamic overlay drawing, it was really nice to have while it worked, but I totally agree that it's not a must-have and everything that goes toward better render performance has my vote anyway, so that's cool. Will be interesting to see if that z offset may need more bumping if we get people with a variety of graphics cards to use it; in my experience there's a chance for a bit of variance on different systems Nevermind, seems to be same offset used in the casing renderer, so should be fine. I suggest making the one with the bump a separate method, so callers can decide for themselves if they need it, i.e. if the caller is sure the things on the same layer don't overlap (e.g. border outline and inner parts)?

ROM/RAM copying - hmm, to make sure we're talking about the same thing: copying the contents of a RAM module on a casing into a ROM module held in hand? That seems to work for me though (sneak rclick = copy to hand, regular rclick = copy from hand). In your branch. Did you fix that since? Am I missing something?

Sturmlilie commented 4 years ago

Nevermind, seems to be same offset used in the casing renderer, so should be fine. I suggest making the one with the bump a separate method, so callers can decide for themselves if they need it, i.e. if the caller is sure the things on the same layer don't overlap (e.g. border outline and inner parts)?

The 0.005 bump by the casing renderer was there before and I highly suspect it's because otherwise the module overlay contents would Z-fight with the casing block. Most of the 1.14 code turned off the depth buffer very liberally, something that is also not available to us anymore. The bumps I do are on the order of 0.001, which I'm can't really confirm is safe for all video cards. I'll dig more into it (we might get Z-fighting when the casing is far away).

Edit: I just tested it by removing the 0.005 bump in the casing renderer, and it indeed ends up in z-fight club for every module, even the ones with just a single sprite. All in all it's a tradeof between guaranteed z-fight avoidance and having the overlay levitate too far above the casing block.

ROM/RAM copying - hmm, to make sure we're talking about the same thing: copying the contents of a RAM module on a casing into a ROM module held in hand? That seems to work for me though (sneak rclick = copy to hand, regular rclick = copy from hand). In your branch. Did you fix that since? Am I missing something?

Apologies, I probably misread the instructions then, I thought just right clicking copied the contents from the installed RAM to the held ROM; it's probably not busted then ^^"

fnuecke commented 4 years ago

Offset: ah, all right then, thanks for testing it!

ROM/RAM: great, one bug fewer to worry about ;)

Sturmlilie commented 4 years ago

Managed to disable diffuse lighting for text / display module without involving Mixins at all :)

image

fnuecke commented 4 years ago

Awesome!

Sturmlilie commented 4 years ago

So, I went through all my commits and made a list of smaller fixes that I'll apply for the next round; if you have any requested changes, those will also go into the next round. I would kindly ask you to create a new 1.15 branch based off master-MC1.14-fabric, then I'll point my PR at it.

I suggest making the one with the bump a separate method, so callers can decide for themselves if they need it

I hope I was able to address this in my earlier comment; basically it wouldn't make sense to make a separate method because every module needs the bump by default; any additional layers go inside that existing 0.005 margin.

Sturmlilie commented 4 years ago

Changes since opening the PR:

Sturmlilie commented 4 years ago

Excuse the churn, I forgot to fix one issue in one the very first commits and that causes all followups to change their hashes during rebase >.>

fnuecke commented 4 years ago

Hey! Sorry for the radio silence there; I'll try to make time to have a look through everything and do a bit of testing this week -- are there any known issues left or are you happy with the PR as it is now?

Sturmlilie commented 4 years ago

@fnuecke I'm happy with the PR as is. I'd like to reinstate the culling of invisible module overlays, but after 1.15 support is in mainline.

Oh, and javadoc comments I'd like to add once you're happy with the code as-is, I hope that's okay. One more thing I just remembered, I never got around to also exposing the new methods I added in AbstractFontRenderer in FontRendererAPI. I'm not quite sure I understand the inheritance tree here; FontRendererAPI is supposed to expose SmallFontRenderer, correct? Which would make implementing something like the Terminal Module from the outside impossible, since it relies on NormalFontRenderer.

Sturmlilie commented 4 years ago

I made a note on one or two places where I got sided annotation warnings, but later I stumbled across a bunch more; too lazy to mark that here, so I'll just add them after the merge.

Hmm, unfortunate that the gradle build task doesn't give me those warnings, I'll see if I can do something about that to save you the nerves.

The Queue/Stack refactor was a bit big (and surprising, since not directly related to the port). I guess it makes sense. Only thing I'm not super happy with is the new closure in the render logic, but I can't think of better (while still readable) way. Unless Java is smarter than I give it credit for and that doesn't allocate each time, then never mind.

Yeah, and I totally expected pushback on it :) After porting the Stack Module, I looked at the Queue Module code and felt weird copy pasting all the rendering code, though it did make me nervous that I ended up having to touch a lot more parts of the classes for the subclassing to work. For the iteration code, I considered a bunch of approaches, first just naively having overridden begin/end methods that return an int index (didn't work due to the queue wrap-around), then using a dedicated Iterator implementation (lots of boilerplate for just one use-site), and settled on the closure due to its simplicity. It certainly allocates once to capture the outer variables, but after I would hope it only invokes the passed lambda without additional overhead.

But I have no problems with reversing the subclassing and just porting the modules each individually, if you're more comfortable with that.

Question: I noticed the GlStateManager methods are all flagged as deprecated now but still used in UI classes. Do you know if the UI code is still "supposed" to be using that, or is there a new system for that it would make sense to switch over to, long term?

From a quick look through 1.15's sources, it appears that most GlStateManager methods marked as deprecated have been superseded by more general methods in RenderSystem. 1.15's internal GUIs also appear to use exclusively Tesselator/BufferBuilder (basically, immediate mode rendering that is as graphics API agnostic as possible) instead of making raw immediate mode GL calls; in fact 1.14 already did so too, albeit still relying on GlStateManager.

I haven't yet looked at 1.16 in detail but I know that all GUI classes get passed an additional MatrixStack for rendering now. I don't know if the Tesselator code is going to stay, but long term any move away from explicit OpenGL is probably a good thing.

Edit: I can try my hand at rewriting the TIS-3D GUI classes to be more future-proof if you'd like, although I'd like to get this mod ready for the 1.16 release first if possible.

Again, thank you so much for the work!

Thanks for taking the time to review, and sorry about not testing this on a dedicated server, I naively assumed that since single player runs on an integrated server instance, it would cover all dedicated paths as well.

Actually I'm surprised to not have gotten push back on a couple other places, eg. the move to vector classes for the infrared packet positioning code.

Edit: oh right, re font rendering: yeah, the API is just for the small font renderer; was originally just intended for text such as on the execution and stack modules.

I figured as much. Maybe the API should expose a method to select one of the different implementations? I feel kind of bad just leaving it broken, but it's up to you.

Sturmlilie commented 4 years ago

Interesting optical illusion I encountered while working on the SequencerModule rendering:
image

The innermost square in the bottom left is brighter than the one at the top.

Sturmlilie commented 4 years ago

Continuing in 2nd PR to avoid clutter: #101