DaFuqs / Spectrum

A full-feature minecraft mod about harnessing the powers of color
Other
102 stars 60 forks source link

Implement bottomless bundle stored item preview #339

Closed imreallybadatnames closed 8 months ago

imreallybadatnames commented 8 months ago

Implements this feedback message. NOTE: This is currently a draft as it does not implement the actual thing currently. However, this PR already introduces a method of rendering items using a custom render function. I'm creating this PR right now in order to receive feedback on this rendering method.

imreallybadatnames commented 8 months ago

image Looks decent to me.

imreallybadatnames commented 8 months ago

Huh, the build check is failing because of ears API not fetching.

imreallybadatnames commented 8 months ago

Not anymore, it seems.

DaFuqs commented 8 months ago

Any particular reason why you imjected CustomItemRender for all items, instead of checking, if the item in question implements it? The rendering itself looks perfect to me. A really nice addition I can see use for other items, too.

imreallybadatnames commented 8 months ago

It's just my brain nagged me about doing a "very costly" instanceof check instead of just calling an interface method for checking custom render support.

imreallybadatnames commented 8 months ago

I mean, it could very easily be ported back into the original design of running an instanceof every item render call, but I just decided to roll with this design without actually knowing which one performs better.

DaFuqs commented 8 months ago

Nothing to criticise about that approach at all, just me being curious. Works for me!

imreallybadatnames commented 8 months ago

Also, one implementation note: the bundle render code relies on smoke and mirrors, e.g. relying on the fact that the projection of the item with current GUI perspective looks just like it was added on top of the bundle sprite, in order to render the stored item properly, which means that it would look very wonky if allowed to render outside of GUI.

imreallybadatnames commented 8 months ago

(also, I probably should remove the supportsCustomRender method given how it is used)

imreallybadatnames commented 8 months ago

... Or not, as I have forgotten that the sole purpose of me adding that function was to slightly reduce render indirection overhead (with shouldRender in ItemMixin serving the purpose of being super called).

imreallybadatnames commented 8 months ago

Well, actually, I think I'm going to gut that method anyway and move the specific function into ItemStackMixin, since it is the one using the appropriate Stack.Extra interface, and as such, I'm thinking of containing that interfaces implementation details purely within that mixin where it belongs.

imreallybadatnames commented 8 months ago

I'll do that by instead adding the boolean allowRecursion(ItemRenderer, ItemStack) for items, where it can be overridden to allow recursively rendering the current customly rendering ItemStack(in the case of the item calling render of its own stack)

imreallybadatnames commented 8 months ago

Actually, I don't think I'll ever need to pass ItemRenderer to allowRecursion as it isn't responsible for actually rendering things. I'll just pass the ItemStack, and also the ModelTransformationMode for parity with shouldRender.

imreallybadatnames commented 8 months ago

Done.

imreallybadatnames commented 8 months ago

Oh, uhh.... I may have done a huge mistake by not actually testing it on a dedicated server.

DaFuqs commented 8 months ago

hehe, I just wanted to note that, too.

DaFuqs commented 8 months ago

(also you should rename your branch to blessedbundle)

imreallybadatnames commented 8 months ago

With the current problem of the test dedicated server crashing at the mixin stage, I think the bundle is very much cursed as of right now.

imreallybadatnames commented 8 months ago

So, some-fucking-how, the CustomItemRender is sneaking into server-side Item class despite the mixin implementing it being only applicable on the client. I don't think it's the injected interfaces, since the wiki pinky promises that it's a compile-time-only feature, and testing that fact would require uglifying the code to a great degree.

imreallybadatnames commented 8 months ago

Yeah, I have absolutely no fucking clue. Better start praying right now to whatever gods there are to get even a slim chance of being blessed with the gift of knowledge.

imreallybadatnames commented 8 months ago

I have removed the (obviously clientside) mixins that reference CustomItemRender, and sure enough, nothing changed. I swear to god, if it's the injected_interfaces thing, I'm going to punch someone.

imreallybadatnames commented 8 months ago

Actually, I think I have an even bigger problem. Can I even implement interfaces only in a certain environment at all? This PR is definitely going to the bin if I don't just refactor the whole thing, and it's definitely going to be a performance-impacting change. Bye-bye clean and seamless interfaces, and hello hash table hell!

imreallybadatnames commented 8 months ago

Unless I devise some sort of plan to filter out client-side interfaces using the mixin plugin, which:

  1. :concern:
  2. I bet would be really hard to do, especially with 0 personal knowledge of ASM
DaFuqs commented 8 months ago

...maybe cursedbundle was fitting, after all

imreallybadatnames commented 8 months ago

Actually, I don't think the aforementioned ASM fuckery would even work without going full-on java agent, considering I'd need to also modify Spectrum's classes to filter out any mention of interface implementation. I'm leaving this mess of a problem for future me(or plainly someone else) to figure out. Surely someone will come up with a good solution, right....?

imreallybadatnames commented 8 months ago

As it stands, there are 2 options:

  1. Go full :concern: and brew up some wicked forbidden wizardry.
  2. Go :faaaaabric: and steal a method or 2 of how FAPI does it.
imreallybadatnames commented 8 months ago

Which boils down to either creating an eldritch horror or getting from a hash table every single render call.

imreallybadatnames commented 8 months ago

Or, what I'm currently thinking of, a secret 3rd option where the implementing item can return its own renderer class instance that's technically an Object but gets casted to the appropriate renderer class in the clientside mixin.

imreallybadatnames commented 8 months ago

That could actually work. Hmmm....

Noaaan commented 8 months ago

I feel like this entire PR could be solved with one Mixin into the ItemRenderer itself. Right now this is really overengineered.

imreallybadatnames commented 8 months ago

Behold.

Seriously though, this thing needs an entirely different approach. As such, I'm closing the PR and deleting the cursed bundle code. Maybe I'll redo it someday, maybe not, maybe someone else does it. Either way, this thing won't see the light of day.