Crash involving The One Probe when placing a logistics module onto a tube
dashkal16 opened this issue · 19 comments
For feature requests, just erase this template and clearly describe the feature you'd like to see
Minecraft Version
1.16.5
Forge Version
36.1.16
Mod Version
pneumaticcraft-repressurized-1.16.5-2.12.2-186.jar
theoneprobe-1.16-3.1.0.jar
Describe your problem, including steps to reproduce it
Crashed when placing a logistics module onto a tube
Any other comments?
Consistent. Trying to re-enter the game crashes again immediately (presumably because I'm still looking at the module)
Is PneumaticCraft making a custom implementation of one of the layout interfaces? And if so why?
It does, yeah. As for the why, it's been a long time, but IIRC I couldn't find an existing implementation or API factory which allowed me to provide a layout style to .vertical() to give the element a coloured border. Posting from mobile right now, but can get more details later.
@McJty Here's the custom implementation: https://github.com/TeamPneumatic/pnc-repressurized/blob/1.16.4/src/main/java/me/desht/pneumaticcraft/common/thirdparty/theoneprobe/TOPInfoProvider.java#L158-L195
and here's where it's used: https://github.com/TeamPneumatic/pnc-repressurized/blob/1.16.4/src/main/java/me/desht/pneumaticcraft/common/thirdparty/theoneprobe/TOPInfoProvider.java#L100
The reason I did it this way is because there is a public API method IProbeInfo#vertical(ILayoutStyle) but no method that I could see in the API to actually get an implementation of ILayoutStyle. Therefore it seemed reasonable to assume that this interface could be implemented as needed, which I did (otherwise what would be the point of a published IProbeInfo#vertical(ILayoutStyle) ?)
If there is an API method to get an implementation, I'll certainly use it.
Looking at the new ILayoutStyle(), it does appear to have decent support in the API for building layout styles now. This is all well & good, but it's incompatible with the previous API (as this issue demonstrates). I think adding defaulted get*Padding() methods would be sufficient to ensure compatibility? Since padding seems to be a new concept, defaulting to no-ops for the setters and returning 0 for the getters should be fine.
Also, you're using AWT now, which is a big no-no for Forge mods. It basically ensures your mod won't run on MacOS.
Downgrading to theoneprobe-1.16-3.0.8.jar resolved the issue. Looks like McJty changed the API in 3.0.8 -> 3.1.0.
@McJty this is due to PneumaticCraft shipping the oneProbe API and getting loaded before OneProbe effectivly deleting the new API changes. Nothing you or me can do about. This was a huge issue in 1.7.10 and YOU SHOULD NEVER SHIP APIS WITH YOU (unless you own the API) (or it could be that it has custom styles that it overrides and now missing functions result is the same)
Not the problem
TL:DR Pneumatic Craft Bug not OneProbe (Still accurate though)
Here is the issue:
https://github.com/TeamPneumatic/pnc-repressurized/blob/1.16.4/src/main/java/me/desht/pneumaticcraft/common/thirdparty/theoneprobe/TOPInfoProvider.java#L158
He extended the API class he needs to implement the new interfaces or extend the "StyleClass" which should be moved to the API anyways
Edit: Ok i skipped way to much.
@desht the Interfaces now provide implementation Access (which makes the API technically moddependent right now)
Because it was impossible to cache without accessing the core. So that was now fixed.
OK, here's the deal:
- In the old API, I created an implementation of
ILayoutStyle. If that wasn't supposed to be done, then a) it should have been documented (it wasn't), and there should have been an API method to create an implementation (there wasn't) - In the new API, there are API methods to get an implementation. That's fine, except for the fact that you have incompatibly altered the API by adding new non-default methods to it. Thus any code that creates an implementation (like mine) now crashes with an
AbstractMethodError. - This could be easily fixed by you defaulting the methods as I previously mentioned to no-ops. Your (non-API) implementation then overrides those to do what it does right now, and code that was built against the old API (like mine) gets a useful default for the new padding features (i.e. no-ops). Then compatibility is preserved.
tl;dr PNC has done nothing wrong here. I've just used the public (pre 3.1.0) API without violating any of its contracts. The breakage has been caused by the changes made to the 3.1.0 API version.
@desht yeah ok. Do you plan to update and fix the crash or do you plan to stay outdated and force basically to mark any new method that is a setter to throw "UnsupportedException" and all getters to return a defaultValue (while the internal implementation just overrides them?)
If you implement a API and that changes you have to make sure that it is supporting new features that can change and can break things in the custom implementation if you don't keep up to date.
That the old api was not good is out of question, but yeah.... Original point stays
Just to give another argument: That would mean that Minecraft/Forge "have to" make sure that Mods don't break on their API interfaces changes. Even if the internal API changes. (Like the switch from Block => BlockState),
No. They "can" do it, but they don't "Have to"
This is about not crashing mods by unnecessarily introducing API incompatibilities. If you don't care about that, please just say so now so I know what to tell users of my mod who now face crashes because of an update to TOP.
Nah, this was a unintentional thing. If i knew that you had your implementation i would either warned you about these changes so you could prepare. Or made sure (if possible because some changes are not catch-able in some scenarios (i am not implying that this was a case)) that this would not have happened. My response was to "We have to know everything that uses us and have to make sure it works" answer, which is like knowing everything on the internet.
But you should really drop the custom implementation in the long run.
In this specific case i will @McJty decide if he wants to default everything new or just await your patch.
So you can tell the people:
API got massively changed, I had my own implementation for things and the guy who introduced the new features missed my implementation and caused this fuckup. ....Your answer how you will deal with it .....
FYI, it was also possible (before the big api changes from @Speiger ) to get access to ILayoutStyle by calling IProbeInfo.defaultLayoutStyle(). That was the intended way to get the layout style in the past. So it was never needed nor intended to implement ILayoutStyle on your own. Granted, this may not have been extremely clear in the TOP API so that was my fault of not documenting that enough. So @desht you should be able to avoid your custom ILayoutStyle even in the older version of TOP. Unless I'm missing something.
I'll add API docs to clarify that you should not make custom implementations of these layout style classes. That's not the intended usage.
Personally I'm not a fan of adding defaults because for some of the new methods it's actually impossible to create a decent default without causing potential issues. Like the new copy() method.
@desht just released a new version of TOP which get rid of awt.Color at least
Yep, we should have a way of keeping compatibility with both pre and post 3.1.0. I'll be testing and releasing a PNC 2.12.3 today hopefully.
theoneprobe-1.16-3.1.2.jar does not resolve the crash.
This was expected as the specific method in question was getTopPadding()
yes, it's expected. A change on PNE's side is also required. @desht is aware of this and told me he would release a hotfix for this soon