Help Box Validation Messages

[daleeidd]

Status: Proposal. Do not merge

The proposal is that the above would compliment current validation approach (OceanValidation) and possibly replace other forms of validation like logging on play.

The benefit is it gives instant feedback to the user and does not pollute the console which people are sensitive to.

There are two approaches to how we target an inspector, and three approaches on how we compose and presenting the help boxes (awkwardly worded but might make sense in a bit).

Being a proposal code quality, naming etc is pretty poor. So please forgive me for that. Mainly to demonstrate the idea to see if we want to pursue further.

Targeting an Inspector

We are using an interface and unfortunately we cannot target interfaces (but we can target through inheritance which is better than nothing).

1. We target the inspector using a custom inspector. We have to create an empty class for each inspector. It is simply two lines which can be appended to ValidatedInspectorEditor.cs (example at bottom of file)
2. We target the inspector using a property drawer. Just by adding ValidatedInspectorProperty (poorly named) as a property, we can target the inspector using a property drawer. See ValidatedInspectorPropertyDrawer.

I'm leaning on option 1 as I think polluting classes with properties for purposes such as this is probably not a good idea or clean.

UPDATE: Went with option 1. I have used the already defined IValidated interface.

Composing and Presenting Help Boxes

I was thinking of a way we could reuse validation code. The validation code should be the same with the only thing preventing it with the current code is DebugLog calls.

1. Basic. We duplicate the validation code. No reuse
2. Advanced. We return a list of structs which have the message and message type and use the appropriate approach (debug log or help box).
3. Functions. We pass a function which wraps either debug log or help box

I am leaning on option two. It allows reuse and works better with adding spaces (before and after help boxes) in the inspector.

Option three looks cleaner but since we have to combine strings for the help boxes but not for debug logs it doesn't work well currently. I would need to add code to collect strings so we can call the help box function once per message type. So it ends up being about the same in complexity as 2.

UPDATE: Went with option 3. Using a shared list since this operation appears to be synchronous. If this is an issue it could be fixed.

---

UPDATE: I have merged in the recent validation code from master. I also went with one approach. Older approaches are in previous commits.

I've used a partial class to help separate validation code from other code. Not necessary though.

Let me know if you think this is worth pursuing further.

[huwb]

Wow this looks great to me, both code and final result - its a million times better getting the immediate feedback in the inspector. I like the partial class pattern. Big thumbs up from me.

What do you think about it?

[huwb]

Ohhh a thing - in the past I avoided wrapping Debug.Log etc because it means you lose the double-click-goes-to-code-line behaviour - it takes you to the log wrapper instead. However i think on 2019+ there are hyperlinks in the callstack so its fair easy to get to the right part of the code, does that sound right?

[daleeidd]

Great. I will finish this off cleanly in the coming days.

Ohhh a thing - in the past I avoided wrapping Debug.Log etc because it means you lose the double-click-goes-to-code-line behaviour - it takes you to the log wrapper instead. However i think on 2019+ there are hyperlinks in the callstack so its fair easy to get to the right part of the code, does that sound right?

I just checked and it does so this will be fine for 2019. Apparently the paid console that lots of people use has that feature too (or something that solves this problem). We won't be wrapping Debug.Log outside of this use case so it shouldn't be too much of an issue for 2018.

[huwb]

Good point. Sounds good!

[moosichu]

Added my feedback, I really like the idea! Sometimes I think it can be really useful to have both log messages and editor-windows. This kind of thing can be an absolute pain to do deal with in automated-build systems -> something went wrong but it wasn't logged but was put in an editor window that no-one can see.. I think by default we should do both, and have the option to restrict it if desired.

[moosichu]

I'm not a huge fan of partial classes, because it means you no longer are guarenteed to have everything related to a class in the same place and you start having cross-references and interdependencies all over the place.

Also, this won't compile in non-editor builds because "CheckShaderName" is now defined in an #if UNITY_EDITOR block, but is still being called from here. I would personally vote to keep this class non-partial. What does @huwb think?

[huwb]

i'll defer to you guys. i like what this aims to achieve, but get your points.

is there inspiration to take from UnderwaterPostProcessHDRP.cs and UnderwaterPostProcessUtils.cs - would pushing out some functions to a separate file work?

[moosichu]

Yeah, the reason I built those in that weird way is because of how different post-processing is in BIRP and SRP, whilst being able to have shared code inside a static utils class. It's not something that I feel super strongly about, just mainly stating why I would probably never make a class partial myself. I'm more than happy to work with them.

[daleeidd]

no longer are guarenteed to have everything related to a class in the same place and you start having cross-references and interdependencies

I would counter with that it helps separate concerns and allows us to exclude validation code from builds. Providing we use it responsibly it should be an improvement overall.

Also, this won't compile in non-editor builds because "CheckShaderName" is now defined in an #if UNITY_EDITOR block, but is still being called from here

Fixed. It would be nice if we could try and keep editor specific code outside of standalone. The concern valid though. If we don't test by building to standalone it is possible to get caught out. But this would be an issue if we wrapped that method with UNITY_EDITOR too.

Happy to concede though and merge them back together.

[moosichu]

The "modern" (not nessecarily better, but could be in this instance) way of doing this would be to put editor-only code inside a seperate asmdef.

https://docs.unity3d.com/Manual/ScriptCompilationAssemblyDefinitionFiles.html

In this isntane it's probably the right thing to do as it prevents editor-code from accidentally being used in gameplay code.

[daleeidd]

Good idea. I'll give that route a try.

[daleeidd]

Partial classes must be within the same assembly. I could only do assemblies for the editors and I think that should be done all at once in a separate PR (there are other editors that could be part of that assembly).

The other issue is that we do call Validate in Awake for some of these components which wouldn't work with an assembly. It isn't mandatory but could be useful to keep that behaviour.

I also had a look at extension methods, but they cannot access private variables. To get assemblies to work we would need to at the very least have to start making more properties public (only getter). It wouldn't be such a bad thing though.

It is possible to do something like the following if you want them merged together:

public class AssignLayer : MonoBehaviour
#if UNITY_EDITOR
    , IValidated
#endif

But this won't protect us from accidentally using editor code in build code.

[moosichu]

Unfortuantely asmrefs aren't a think in 2018.4 otherwise I would say to use that instead.

I will hav a look at the PR later this weekend :)

[daleeidd]

I had a look at asmrefs but I couldn't see how they would help.

[daleeidd]

Thanks both.

I did a rewrite since it was pretty messy as a proposal. I didn't see the review until I was finished. Sorry, I should have marked it as draft. I have incorporated feedback.

[huwb]

Can we get this merged? (what do the remaining steps look like)

[daleeidd]

I just need to merge master in. I was waiting on some feedback about partial classes, but I will just go ahead as we can always change it later if it becomes an issue. Will be ready tomorrow.

[daleeidd]

@huwb This is ready to merge. I went through and tested a fair bit including building the main scene and I haven't experienced any issues.

[huwb]

nice!

fwiw while i have limited experience and see the potential downsides, i am ok with this use of partial classes, particularly as the separation is fairly clean (runtime vs editor). so my vote would be to get it in and we can change it later.

[moosichu]

Looks good to me!

[daleeidd]

Thanks all. Merged.

[huwb]

Awesome!

@daleeidd minor thing but while messing around in windows editor i found when the help box appears it yanks focus away from the current input for me. So for example i cant easily type a layer name into the OceanDepthCache component (have to type it one character at a time).

I could fix this by moving the call to ShowValidationMessages() to the end of the inspector. This also means that the fields do not jump down when the message appears which i find less jolting if i'm looking at the property while typing, so i think i'd vote for this. What do you reckon?

I guess one more thing i just found on the oceandepthcache - if the layer name is empty it does not show a validation box (but does report a message to the log). probably a simple fix but i need to run now so thought i'd report it here

[daleeidd]

Thanks. Fixed. That is much better for UX.

And thanks for the second one too. Fixed. That log wasn't part of validation and I suspect there will be a few more like this still. There might be a few things to iron out in relation to running in edit mode too still. But nothing major.