Improve structure error feedback

[RecursivePineapple]

This adds a mechanism for easily adding validation error messages for unique structure requirements. Many multis have specific requirements, but these requirements aren't communicated anywhere and there's no feedback for when the requirements aren't met. This PR allows multis to add error messages easily.

For now, it's only implemented on the cryo freezer and gt++ multis, but if this is accepted other multis can add their own validations easily.

[RecursivePineapple]

Drafting because I didn't know someone worked on something similar (just saw the merge conflicts). Let me see if this needs to be closed first.

[RecursivePineapple]

Some examples:

[RecursivePineapple]

False alarm, I saw get/set DisplayErrorID and thought it was a similar mechanism but it looks like that's just for generic errors shared by all multis.

[YannickMG]

With this current implementation it looks like each implementation will need to define exactly how it wants to handle displaying a given error message, and that when said error message is displayed the data needed to explain the error may need to be reacquired if the StructureError isn't specific enough.

Letting each implementation decide how to display an error is great, it would make it simple for a given implementation to ditch the text display and use other UI elements to indicate the error instead.

The fact that the data relating to the error needs to be re-acquired is a little bit inconvenient though. It works fine for these simple cases, but I can see this feature being extended later to tell you, for instance, that an invalid block was found at position X, Y & Z during the structure check. In order to display a helpful error message we would effectively need to scan the structure again at render time. It might be helpful if these errors could contain the information needed to render them.

Now I see you've given this some thought since you labeled mStructureStatus a list of "unparameterized" structure errors. What was your reasoning for foregoing parameters and state? I recognize that using EnumSet requires foregoing parameters but holding information on why a structure check failed shouldn't be on any kind of hot path that would require it.

[RecursivePineapple]

@YannickMG It's unparameterized mainly because of syncing. The structure check always runs on the server, while the text code always runs on the client. It's trivial to sync an EnumSet since you can easily convert it into a byte array, but trying to sync generic data would be way too complex when all that's needed in the multi is a few FakeSyncWidgets. Maybe you could do something with syncing a json object, but it would still be overkill.

A good example is the casing count. Multis may have one 'main' casing, or they could have several in more advanced structures. If my assumption turns out to be incorrect, and we only need to sync a couple of very simple variables, then we can add another layer or refactor this, but I don't want to make that assertion for now.

[YannickMG]

Some structure errors are emitted despite not matching a structure requirement.

Perhaps you could move the code to add a specific StructureError to be part of the structure definition itself so that there can be no mismatch. Otherwise you'll need to find some other way of ensuring validateStructure doesn't need to be overridden by every multi with different requirements, unless of course that's the intent.

[YannickMG]

@YannickMG It's unparameterized mainly because of syncing. The structure check always runs on the server, while the text code always runs on the client. It's trivial to sync an EnumSet since you can easily convert it into a byte array, but trying to sync generic data would be way too complex when all that's needed in the multi is a few FakeSyncWidgets. Maybe you could do something with syncing a json object, but it would still be overkill.

A good example is the casing count. Multis may have one 'main' casing, or they could have several in more advanced structures. If my assumption turns out to be incorrect, and we only need to sync a couple of very simple variables, then we can add another layer or refactor this, but I don't want to make that assertion for now.

That makes perfect sense. EnumSets are just fine in this case and it would be problematic to try to anticipate the needs of future parameterized errors in terms of network packets.

[RecursivePineapple]

Otherwise you'll need to find some other way of ensuring validateStructure doesn't need to be overridden by every multi with different requirements, unless of course that's the intent.

Yes, that's the main goal. I figured it would be the simplest way to validate the structure. Most of the structure definition code is provided by structurelib, so I can't add to it easily. I have an in-progress project to help with this, but it wraps structurelib completely (it hasn't been drafted yet but I've used for several projects and it works very well - I want to iron out all the kinks first).

I could add some sort of List<Predicate<T extends MTEMultiBlockBase>> mStructureValidations mechanism to all multis, but I thought that would've been overkill when it could easily be replaced by a simple method call.

I hadn't expected steam multis to not require maintenance, I'll fix those. In cases like this, you would either change the superclass to require a boolean field for maintenance hatches, or you'd just not call super.validateStructure().

[YannickMG]

This code doesn't appear to be foolproof since a machine may simply fail to add encountered hatches to mMufflerHatches.

        if (this.getPollutionPerSecond(null) == 0 && !mMufflerHatches.isEmpty()) {
            mStructureErrors.add(StructureError.UNNEEDED_MUFFLER);
        }

What's an example of a machine it does work for?

[RecursivePineapple]

I'm not entirely sure what multis use it, it's just what checkHatches used to do so I kept it. Almost every gt++ multi emits pollution so it probably isn't that useful, but I didn't want to remove it without a good reason.

[YannickMG]

Fair enough. This will require additional work on several multis to get the most out of it, but it is a nice capability to have.

[miozune]

Regarding syncing parametrized errors, see IMachineMessage which is implemented by CheckRecipeResult and ShutDownReason. Making use of it would allow multis to display more detailed information about error.
While current code is great as a starting point, I believe we can use more refinement before going implementing all other multis.

Also, as pointed out by Yannick, having duplicated code for checking structure can easily lead to bugs. StructureLib code is not something we can hook into right now, but at least we can control other checks. I'd imagine something like this would be great: Nvm

```java // Specific to multi List validateStructure() { List errors = super.validateStructure(); reset(); if (!checkPiece()) { // We can't access StructureLib internals, but at least we know something failed with checks errors.add(StructureError.incorrectStructure); } if (mCasing < 10) { // We know exactly what went wrong errors.add(StructureError.tooFewCasings(mCasing, 10)); } return errors; }

// MultiblockBase
boolean checkStructure() {
List errors = validateStructure();
this.structureErrors = errors;
this.mMachine = errors.isEmpty();
return mMachine;
}

</details>

[RecursivePineapple]

I made the mStructureErrors field private so that we can refactor the system easier if needed and added it to the parameters of validateStructure and getStructureErrors. I also added an NBT parameter so that multis can add arbitrary data (it's synced properly, though it has a bit of latency).

I thought about adding some sort of interface system to hide some implementation details, but it would introduce too much boilerplate for not much gain (a bit easier to refactor and namespacing for the nbt data, which won't be a problem). There aren't any limitations to the current system that I know of (though I will admit it's not the prettiest). I think the simplicity makes up for its lack of prettiness (I'm not a fan of excessive boilerplate).

Something like this:

interface IServerErrorStorage {
  void addError(StructureError error);
  
  void addError(StructureError error, NBTTagCompound data);
}

interface IClientErrorStorage {
  boolean hasError(StructureError error);

  NBTTagCompound getErrorData(StructureError error);
}

[miozune]

I'm still not entirely convinced by NBT syncing v.s. IMachineMessage approach, but it's true that NBT can carry flexible information without requiring distinct class, which would the case for structure errors. I'd like to approve the idea now.

Here are some comments need addressing before merge. Sorry for the late review!

[miozune]

We should avoid introducing m prefixes for new members, so structureErrors

[miozune]

Same here

[miozune]

Suffix Impl refers to a method which child classes override. The name should be swapped so that child classes will override validateStructureImpl.

Also, since the intention is to not let children to override this method, it should be marked as final. Keeping protected is ok so that it can be called from anywhere.

[miozune]

SideOnly usages should be restricted where it would cause crash if not present. It can be removed.

[miozune]

You shouldn't rely on a field being in {@code data} if an error is present.
Can this be clarified a bit more? Especially whether "error" refers to errors argument or general word is unclear.

[miozune]

I believe it's worth creating dedicated class extending FakeSyncWidget, since how StructureError gets synced has nothing to do with multiblock.

[miozune]

What's the intention behind the change to the constructor? (besides mFuelStack and CASING_TEXTURE_ID removal) I mean it works, but also inconsistent with all other MTEs. (Yes MTE system is annoying, I'm just not sure if we want to mix two styles for newMetaEntity implementation.)

[miozune]

Can this have better naming? Having structureErrors and structureErrorData looks confusing to me. I'd suggest structureErrorContext. And data argument in validateStructure and getStructureErrors can be also renamed to context if you like this idea.

[miozune]

"Get" sounds too generic when it's called solely on client, gathers error strings rather than StructureErrors, and returns nothing. I'd suggest gatherStructureErrorMessages, addStructureErrorMessages or buildStructureErrorMessages.