AssetReferenceUIRestriction should be in the Runtime assembly

AssetReferenceUIRestriction as well as AssetReferenceUILabelRestriction shouldn't be in the Editor assembly. I don't mind having to wrap all my uses of these attributes with #if UNITY_EDITOR but forcing my runtime asmdefs to depend on the Unity.Addressables.Editor.asmdef is really backwards and awful.

 

It's not pretty, but I dislike the alternatives more.

1. move it to the existing runtime assembly. We can't do this because our editor assembly depends on (and needs to depend on) the runtime. So the runtime can't depend on the editor assembly. No circular dependencies.
2. create a new assembly, called something like Unity.Addressables.EditorButNotEditor that depends on both existing ones. This would mean you would link your asmdef to something other than our editor assembly. And it could mean that you could avoid wrapping your attributes in #if UNITY_EDITOR, but we couldn't actually make those attributes work. AssetReferenceUILabelRestriction can't do it's restriction without editor-only data. And I don't particularly want to provide a class that exists in a runtime world, but doesn't actually work.

So I don't love the situation, but I don't love the alternatives either.

 

If it was possible to conditionally reference the Addressables.Editor asmdef only for the Editor target I would be fine leaving it. But right now it must try and fail to load the Editor asmdefs in standalone builds too.

 

Oh, so you have some assemblies you are defining within your project? I was thinking you just had the main code, which is in Assembly-CSharp. That assembly (as far as I'm aware) dynamically changes dependencies depending on if it's in editor or not.
but if you have an asmdef, i'm not sure how to deal with that. How are you getting that to work? i'd like to do some tests around it myself, but if you have it working (even in an annoying and painful way), I'd love to see it.

 

Yes my entire project is in asmdefs (27 total). I have no code that is in the default assemblies. I have in fact another layer of pain ontop where I have a local package full of addressable extensions which has 2 additional asmdefs (a Runtime and an Editor asmdef ror the extensions). I have a

public sealed class AssetReferenceUIScriptRestriction : AssetReferenceUIRestriction

in the Editor extensions which I use in my game code asmdef to make it easier to choose the right addressables for an AssetReference property (these attributes are wrapped in #if UNITY_EDITOR when used).

All this said means I have a runtime Game.asmdef which must depend on Unity.Addressables, Unity.Addressables.Editor, AddressableExtensions, and AddressableExtensions.Editor.

Actually I take this back, I think this was a misunderstanding on my part (part of an unrelated error). It looks like it just silently ignores the asmdef reference which is fine.

 

that's good news. thanks for the update and explanation. Sounds like quite a system to manage!

 

I may be missing something here but I do have to concur, why are these in the Editor assembly? This is not the standard pattern for Editor attributes. The Editor payload code of the attribute should be on the editor side, in this case the AssetReferenceDrawer, not in the attribute class itself.

Single Responsibility Principle and Separation of Concerns are not really doable here, they never have been when it comes to custom property drawers and the like, trying to force them on editor code is causing this awkwardness. No other attribute that decorates runtime UnityEngine.Object fields in the entirety of the Unity assemblies is defined in an Editor assembly. To back this up, look at how UnityEngine.TooltipAttribute, UnityEngine.ContextMenuAttribute etc. are implemented by UnityEditor.PropertyHandler.

In our projects, it's considered an antipattern to have anything UnityEditor.* in a runtime script unless absolutely necessary. I get how it's nice to be able to cleanly derive UI restrictions from that attribute but I feel it would be far more idiomatic Unity code that works like everything else to split the implementation of that into the property drawer itself in a pattern matching switch.

 

Those are some good points.
The root class AssetReferenceUIRestriction could be moved to the runtime assembly. But our concrete version AssetReferenceUILabelRestriction can't because it needs access to editor info. I had put the root in the editor assembly so they could be together, but on thinking about it, i do see the value in moving the root restriction out. That would only help those creating custom restrictions. I'm not sure if anyone in that bucket exists.

 

But the thing is, it only needs access to Editor info because its implementation is in the attribute class itself. That’s the bit that is not typical, mainly. If the attribute itself was in the runtime class and it had its implementation in an Editor-side surrogate, it would be perfectly possible to have that in the runtime class. You could associate the surrogate with the runtime side by marking it with an attribute, the same way that property drawers get marked with CustomPropertyDrawerAttribute.

Something along the lines of this pseudo code I mocked up on my phone:

Code (CSharp):

1. public class AssetReferenceUILabelRestriction : Attribute
2. {
3.     public string SomeAttributeData;
4. }
5.  
6. //Then in the editor assembly:
7. public class AssetReferenceSurrogateAttribute : Attribute
8. {
9.     public Type TargetType;
10.  
11.     public AssetReferenceSurrogateAttribute(Type type)
12.     {
13.         TargetType = type;
14.     }
15. }
16.  
17. [AssetReferenceSurrogate(typeof(AssetReferenceUILabelRestriction))]
18. public class AssetReferenceUILabelRestrictionSurrogate : AssetReferenceRestrictionSurrogate
19. {
20.     public override bool SomeOverride(AssetReferenceRestrictionAttribute attribute)
21.     {
22.         return (attribute is AssetReferenceUILabelRestriction typedAttribute) && typedAttribute.SomeAttributeData == “blah”;
23.     }
24. }
25.  

Then in the actual property drawer when you get the properties, you can use reflection or the TypeCache API to get the surrogate for the encountered attribute, get (and cache) an instance of it with Activator.CreateInstance() and use it. For an example of this look at the implementation of PropertyHandler in the Editor itself. This pattern is designed to tackle this exact use case.

It fulfils both the criteria of separation of runtime and editor implementations and the ability to extend the system without modifying package code.

I can knock up a working version and post a patch for reference, if it’d help.

 

I agree, that is a better model. You have convinced me to add this to the list. If you want to whip up an example, that'd get us going quicker, but if not, we'll get to it.

Thanks for the added detail of explanation.

 

I'm still completely lost why it was decided that it was OK to put the AssetLabelRestriction attribute in the UnityEditor to begin with. Contrary to the original user, putting an #if/else around every single asset reference that we want to restrict to labels is beyond tedious. I'm glad that you're redoing it, but disappointed that such an important editor workflow was determined to be good enough with a ton of undesirable if/else in the code.