Draconic-Inc / Draconic-Evolution

Other
325 stars 165 forks source link

Can't initiate DE infusion crafting while wearing Tinkers Chestplate #1653

Closed Rohmad96 closed 2 years ago

Rohmad96 commented 2 years ago

This is the most bizarre, unexpected glitch I've ever come across. Isolating this was a nightmare, and I couldn't tell you why this happens, but it does. Please let me know if this is a Tinkers issue instead.

Base information:

Description / steps to reproduce

  1. Create a tier 1 fusion injector setup into a core
  2. Wear Traveler's Vest from Tinkers Construct
  3. Try to initiate crafting (e.g. the Wyvern injector recipe) Regardless of power input or not, it won't even get to the "charging" text. Clicking the "Craft" button just does nothing.

Please, look into this to prevent anyone else the kind of absolute headache I just went through. Thanks!

javaw_vEUPEUJK50

FoxMcloud5655 commented 2 years ago

https://github.com/SlimeKnights/TinkersConstruct/issues/4838

KnightMiner commented 2 years ago

Any update on this? It makes no sense to me that armor would break the UI, if there is something broken in the armor code I need to know so I can fix it, or if this is an issue on your side that is useful information as well.

brandon3055 commented 2 years ago

@KnightMiner Just did some digging. If a player is wearing your chestplate you block all RightClickBlock events. https://github.com/SlimeKnights/TinkersConstruct/blob/1.18.2/src/main/java/slimeknights/tconstruct/tools/logic/InteractionHandler.java#L180 If a player does not have permission to right click one of my tiles then they do not have permission to send packets to that tile.

The reason i do this is because without it, it would theoretically be possible to bypass grief prevention by sending a packet with a modified client. And such a packet could theoretically be used to detonate a DE reactor.

Its hard to say who is in the wrong here. My implementation is a little unconventional but it seems fair to assume that if an event is canceled then the interaction should be blocked. On the other hand you are claiming exclusive control over the end of the event. Yours is the kind of code that can only be implemented by one mod because if someone else were to try to do the same thing it would conflict with yours.

KnightMiner commented 2 years ago

There is a big difference between a canceled right click event and a player not having permission to perform an action, I disagree that is a fair assumption as that is the whole reason for the cancelation result. If you are going to base your behavior off the event being canceled, you should check the result to see if its a local "you have no permission" result, that is FAIL. Otherwise you will get false positives for anyone who adds an interaction during the event that happens to fire at your block. Likewise, firing the event can have side effects, how do you deal with the case where a modded item cancels the event when its in the main hand?

For a cleaner approach, why not just check if the player is in your container serverside? There is no way for them to be in the client side UI (without hacks) unless the serverside UI is open. If you enforce the serverside UI only opens via the serverside right click block logic, you bypass the possibility of a hacked client opening it at the wrong time.


As far as claiming exclusive control over the event, that is why I only run my logic when you are wearing my chestplate and have an empty hand, and additionally I run it at low priority. Its unfortunately necessary to implement the logic in that way as forge does not fire enough events on right click for me to otherwise properly implement the relevant interaction hooks, no good way to change that without mixins (and if we are considering mixins to be a valid approach to the problem, one could simply mixin Level#mayInteract for their grief protection making that method sufficient for your needs)

brandon3055 commented 2 years ago

For a cleaner approach, why not just check if the player is in your container serverside?

You are correct here. I was talking to a friend about this issue last night and we came to the same conclusion. I just dont know how i missed this when i was implementing my system in the first place. I guess i was just so focused on replacing vanillas built it packet system with something better that i missed the one useful feature it has.

I will be implementing this and removing my event based system in the next build.

FoxMcloud5655 commented 2 years ago

I'll note this for my mods if I ever need to do the same. Good to know.

brandon3055 commented 2 years ago

This is now fixed in a new release of BransonsCore