Generator rewrite #281

CaseyHofland · 2021-06-09T10:45:58Z

Boy oh boy is this a doozy.

The generator has been rewritten to work with types.

This works brilliantly as you may now perform:
Generator.Generate(typeof(AtomEvent<float>), myPath, myNamespace);
And things will just work.

The generator accepts a delegate of type Resolver.

string Resolver(Type type, out string className, string withNamespace = default)
The coup de grâce of this is the static Template.ResolveAtom function which generates the atoms entirely using strings (I WOULD use CodeDOM for it but nooooo, that isn't supported by .NET Standard 2.0).
This change means that the templates in the resources folder are no longer needed.

Resolvers are superuperduper easy to extend.

public static string ResolveConstant(Type type, out string className, string withNamespace = default)
{
    type = typeof(AtomBaseVariable<>).MakeGenericType(type);
    return Template.ResolveAtom(type, out className, withNamespace);
}

This can then be used like so:
Generator.Generate(typeof(float), myPath, ConstantTemplate.ResolveConstant, myNamespace); same as:
Generator.Generate(typeof(AtomBaseVariable<float>), myPath, myNamespace);
The gain here is that if you have multiple resolvers, you can pass in the same type for all of them.

The AtomGenerator options are dynamic and extendable.

Currently the AtomGenerator has options for Constant, Event and Pair Event. These aren't hard coded in: they are retrieved from a static dictionary of resolvers. Say you want to add a new generator. All you have to do is:

[InitializeOnLoad]
public static class ConstantTemplate
{
    static ConstantTemplate()
    {
        AtomGenerator.resolvers.Add("Constant", ResolveConstant);
    }

    public static string ResolveConstant(Type type, out string className, string withNamespace = default)
    ...
}

This automatically adds the option in the AtomGenerator, thus making extending the AtomGenerator as easy as using it!

In light of these improvements I have deprecated the old generator methods.

This is currently a draft, as it is not yet up to feature-parity with the old generator. For some of the smaller details I would refer you to the changelogs.

The methods take a type and a resolver. The resolver is used to generate the atom, the type is used to find what values to put in. This modular approach makes it incredibly easy and straightforward to add new resolvers, as can be seen in the Generator class on line 208, where Template.ResolveAtom is given as a default value.

The AtomGenerator has been made up to date with the Generator Window. - All atom scripts are now created next to the AtomGenerator instead of in different folders... - Unless the scripts have been and are tracked by the AtomGenerator. - It supports defining a namespace. This namespace WON'T automatically default to UnityAtoms anymore. - The option buttons now work. - Options are based on the resolvers inside AtomGenerator.resolvers. Adding to / removing from this dictionary is handled cleanly by the editor. - Undo opperations for every value. 3 Resolvers have been implemented for testing (and the other ones won't take a lot of time to add). One for an AtomBaseVariable<>, one for an AtomEvent<> and one for an AtomEvent<Pair<>>. The TypeSelectorDropdown has earned its own seperate file. It has been moved out of the AtomGeneratorEditor, together with the NamespaceLevel, which is now a child struct of TypeSelectorDropdown.

CaseyHofland · 2021-06-09T10:54:07Z

Mentioning #61 since the Resolvers extendibility could probably solve this issue, but it would need to be experimented with.

CaseyHofland · 2021-06-09T10:54:50Z

Also mentioning #60 for the same reason as above.

CaseyHofland · 2021-06-09T10:58:17Z

Mentioning #235 because this update should totally make the AtomGenerator good for it.

The new generator system doesn't use them.

The AtomGenerator can now reliably track your options. This is important because if a template is added or removed, the generator needs to stay intact. Furthermore, if a new template is added, by default it must not be included in a generators options, unless the generator is newly created.

- AtomGenerator throwing an unnecessary warning. - Resolver delegate wasn't inside the UnityAtoms.Editor namespace.

TemplateData has been separated from the Template class. It makes the CreateAssetMenu attribute optional and accepts a methodString if a Template wants to add custom methods, as is demonstrated in the EventTemplate. An extra UX change has been created made where the class name will no longer but the first of its generic inheritances last. Something inheriting from AtomEvent<Pair<bool2>> would first create the class name Bool2PairAtomEvent, which now becomes Bool2AtomEventPair.

CaseyHofland · 2021-06-11T23:37:20Z

Creating the templates for the new system is underway, but I will need to test if they are created exactly like the old ones.

CaseyHofland · 2021-06-11T23:46:29Z

I have a question regarding the naming of atoms. Currently I generate as much as I can just from the Type you are generating from. For something like Constants, this leads to class names like Bool2AtomBaseVariable, since what we call a Constant is actually the class AtomBaseVariable.
I do prefer this method since I think it makes it clear where a generated atom originates from and incentivizes using good naming conventions. Then, it would be better I think if we rename types like AtomBaseVariable to simply Constant, which will generate the desired name Bool2Constant. Remember, it's still in the UnityAtoms namespace, so its full name is still UnityAtoms.Constant. Would this solution be acceptable?

miikalo · 2021-06-12T15:34:22Z

I have a question regarding the naming of atoms. Currently I generate as much as I can just from the Type you are generating from. For something like Constants, this leads to class names like Bool2AtomBaseVariable, since what we call a Constant is actually the class AtomBaseVariable.

Why wouldn't it become UnityAtoms.Bool2Constant? I'm not sure I follow...

I do prefer this method since I think it makes it clear where a generated atom originates from and incentivizes using good naming conventions. Then, it would be better I think if we rename types like AtomBaseVariable to simply Constant, which will generate the desired name Bool2Constant. Remember, it's still in the UnityAtoms namespace, so its full name is still UnityAtoms.Constant. Would this solution be acceptable?

I like the idea of simplifying names, but even though Constant does not add any functionality to an AtomBaseVariable, either of them might change later for different reasons, and that's why I wouldn't mind being a bit more explicit. How about UnityAtoms.BaseVariable?

miikalo · 2021-06-12T15:42:14Z

Nitpicking, but:

This was kind of confusing, not sure why it says "Mono Script" and I wasn't sure what to select for it, until I clicked Generate and the generated Atom appeared in it's place. When you first see the generator it's confusing. Could it be a read-only field but it would still take you to the generated asset when clicked?

CaseyHofland · 2021-06-12T17:06:21Z

Could it be a read-only field but it would still take you to the generated asset when clicked?

Sure thing! I’ve experienced some confusion from it myself and this is actually the perfect solution.

CaseyHofland · 2021-06-12T17:12:17Z

Why wouldn't it become UnityAtoms.Bool2Constant?

I've changed a few things here and there with the generator.

1. No enforced namespace.
The old generator would always put generated atoms under the UnityAtoms namespace, but I don't think this should be enforced as it constricts the generators flexibility. Plus removing it from the generated files is tedious, while adding it is painless.
If it is preferred, I can always make the AtomGenerator use UnityAtoms as the default namespace, whilst remaining completely editable.

2. Class name from type.
Generated atoms get their class names from the granted type. What we refer to as a Constant, in code that is actually the type AtomBaseVariable. So the class name becomes Bool2AtomBaseVariable. However:

I like the idea of simplifying names, but either of them might change later for different reasons,

You make an excellent point here. If ever a class name would change, immediately all our user's code would break. So I should probably accept a name parameter 😅

miikalo

Great job, the bulk of work seems to be out of the way with this!

Packages/Core/Editor/Generator/Generator.cs

Packages/Core/Editor/Extensions/TypeExtensions.cs

Packages/Core/Editor/Generator/AtomType.cs

Packages/Core/Editor/Generator/AtomRecipe.cs

Packages/Core/Editor/Generator/Generator.cs

Packages/Core/Editor/Generator/Templates/Template.cs

- Fixed a typo in AtomRecipe. - Deprecated the type methods working with the legacy generator. - Set the default namespace of the AtomGenerator to "UnityAtoms". - Made monoscript fields of the AtomGenerator disabled for editing.

Since there is no longer a standardized "default template" the generate method that did not require a Resolver has been removed.

CaseyHofland · 2021-06-21T15:49:13Z

Quick update on plans:

This generator merge will likely not finish it yet. I've taken another good look at my goal, and what is very important to me is that the generator needs to be able to generate all the atoms in base atoms on its own.

This creates a bit of a catch 22 because that is currently not possible. Base atoms still needs a few updates here and there for the V5 change, which will thus influence the generator as well. However, I think it is best to take it as far as it can for now, merge, and update it later.

Plus, now that the generator is more general, I want to experiment with creating e.g. a ClampGenerator, but this is no priority of mine and I might leave it for after 5.0.0.

Packages/Core/Editor/Generator/Generator.cs

miikalo · 2021-06-21T21:11:25Z

Good work so far, just request a re-review when ready.

Templates don't rely on anything else anymore: they can be used to generate script templates all on their own by filling out their parameters and resolving them.

The generator is now itself an abstract ScriptableObject that contains all the necessary data to generate scripts (previously this was held by the AtomGenerator). Its Generate methods also use the new Template class for generation now. This change should make it easier to derive from the Generator and create new types of Generators easily. The type selectors' safe search option has been upgraded to include special case Unity Serializable types.

EditorIconPostProcessor wouldn't work if it was fully typed (UnityAtoms.EditorIcon).

- Is equatable check would always return false. - If a new generator is created, Script Compilation is requested to ensure templates being added through InitiliazeOnLoad.

This will add all the templates to the AtomGenerator. They have feature parity with the old generator, save for summaries.

Generators didn't serialize correctly when Unity was started. Note that there wasn't any way to fix the MonoScript serialization. They have been removed from the Generators display to not cause any confusion.

miikalo

Looks good, really like the ideas presented in the video. Not going to approve before the PR goes live.

miikalo · 2021-07-04T09:00:07Z

Packages/Core/Editor/Generator/Templates/ConstantTemplateFactory.cs

+namespace UnityAtoms.Editor
+{
+    [InitializeOnLoad]
+    public static class ConstantTemplateFactory


All these factories seem to share the same methods, but they don't implement an interface or force the methods in any other way either. Any solutions for this in C#? Do these need to be static?

The way I implemented it, the GetTemplate methods need to be static in order to be added dynamically to the AtomGenerator (in hindsight, perhaps ScriptableObjects carrying Templates would have been better).
Since everything in the Factories was static and I didn't want them to be instantiatable, I opted to make them static.

miikalo · 2021-07-04T09:01:47Z

Packages/Core/Editor/Generator/AtomGenerator.cs

            {
-                GenerationOptions |= (1 << i);
+                var generatorGuids = AssetDatabase.FindAssets($"t:{nameof(AtomGenerator)}");


This part repeats, as well as the loop below it. Extract to private methods or move to base class.

Good point, I will make it a private method (the Generator needs to stay clean of this since the AtomGenerator uses this as a kind of "convenience event": no need for the Factories to repeat this FindAssets method every time they want to add their Template.

miikalo · 2021-07-04T09:03:20Z

Packages/Core/Editor/Generator/Templates/PairTemplateFactory.cs

+
+namespace UnityAtoms.Editor
+{
+    public static class PairTemplateFactory


Why does this one look so different from the others?

Instead of writing a GetPairTemplate method for every pair, I instead wrote a GetTemplate method that modifies an existing GetTemplate method to turn that template into a pair. That may be pretty confusing, but just look how it is used in the EventTemplateFactory. It generates both an AtomEvent and an AtomEventPair from the same GetTemplate method, but one is parsed through the PairTemplateFactory.

miikalo · 2021-07-04T09:05:22Z

Packages/Core/Editor/Generator/Templates/VariableTemplateFactory.cs

+                {
+                    body =
+$@"
+        /// <summary>


What are these methods? I don't remember them...

They were in the original .txt templates. Under certain conditions, the generator will create methods for the generated AtomVariable as well.

miikalo · 2021-07-04T09:08:09Z

Packages/Core/Editor/Generator/ClassModifier.cs

@@ -0,0 +1,9 @@
+namespace UnityAtoms.Editor
+{
+    public enum ClassModifier


Virtuals, Partials, how many need to be here?

Partial would be a different kind of modifier, since a class can be "partial sealed" or "partial abstract".

Keep in mind that "public, private, protected" and similar would be an access modifier. I didn't write this whole generator system out as much since we don't need all that for Atoms and since actually writing out such a system would take a lot of work getting the details right.

Packages/Core/Editor/Generator/Templates/VariableTemplateFactory.cs

Also added an optimization where the generators are cached for 1 Editor loop so the expensive FindAssets method isn't repeated for more than once if it's called multiple times in the same Editor loop (which is very much the case with the Generators' implementation).

- This is implemented for the TemplateFactories. - TypeExtensions' GenericName method has been renamed to CodeCompatibleFullName. - TypeExtensions has a new method called CodeCompatibleName. It creates a code compatible string out of a type, but unlike CodeCompatibleFullName it doesn't include the namespaces.

CaseyHofland · 2021-07-06T15:44:10Z

The summary option has already been tested and generates the exact same summaries as the old generator.

AdamRamberg

Really great job on this PR! See my comments for some stuff that needs to be addressed before we merge. Other than the comments we also need to create generators for all generated types in all sub packages (BaseAtoms etc). This was handled by the old generator and needs to be in place before we merge (or at least before release of v5).

AdamRamberg · 2021-07-07T03:04:42Z

Packages/Core/Editor/Generator/Generator.cs

+        /// <param name="directory">The folder path where the scripts are going to be written to.</param>
+        /// <param name="template">The template to generate the script from.</param>
+        /// <param name="templates">The templates to generate the scripts from.</param>
+        public static MonoScript[] Generate(string directory, params Template[] templates)


This function is not used. Remove it.

I would like to keep it in there as a general method. The idea behind it is that if you have a Generate(string, Template) method somewhere and ever want to extend it, you can just do Generate(string, Template, Template, Template) and it will work.

Alright, that is fine

AdamRamberg · 2021-07-07T03:09:20Z

Packages/Core/Editor/Extensions/TypeExtensions.cs

+            RecursiveGenericName(type);
+            return typeName.ToString();
+
+            void RecursiveGenericName(Type type)


I'm getting a compile warning here:
error CS0136: A local or parameter named 'type' cannot be declared in this scope because that name is used in an enclosing local scope to define a local or parameter
Rename the variable to something like innerType

AdamRamberg · 2021-07-07T09:20:48Z

Packages/Core/Editor/Editors/GeneratorEditor.cs

+                EditorGUILayout.PropertyField(option, optionLabel);
+
+                EditorGUI.BeginDisabledGroup(true);
+                //EditorGUILayout.PropertyField(script, GUIContent.none);


Should be uncommented right?

Ah no, should be removed. I still need to take out the broken MonoScripts from the Generator.

Why are MonoScripts broken? Not sure if I've missed a previous discussion?

I do some weird stuff in OnBefore- and OnAfterSerialization which breaks it unfortunately.

AdamRamberg · 2021-07-07T19:01:58Z

Packages/Core/Editor/Generator/Templates/AtomTemplateFactory.cs

+        {
+            var template = GetTemplate(atomType, typeArgument, atomName, editorIconName);
+
+            template.attributes.Add(typeof(AddComponentMenu), new[] { $"\"Atoms/{template.className.Remove(atomName.Remove(" "))}/{atomName}\"" });


the last /atomName is not enough here since this is going to be the display name of components added to GameObjects (tests it yourself). I would suggest the last part to be template.className, but split on camel case (something like https://stackoverflow.com/a/6137889).

AdamRamberg · 2021-07-07T19:02:53Z

Packages/Core/Editor/Generator/Templates/AtomTemplateFactory.cs

+            var template = GetTemplate(atomType, typeArgument, atomName, editorIconName);
+
+            template.classModifier = ClassModifier.Sealed;
+            template.attributes.Add(typeof(CreateAssetMenuAttribute), new[] { $"fileName = \"{template.className}\"", $"menuName = \"Atoms/{template.className.Remove(atomName.Remove(" "))}/{atomName}\"" });


Make sure that the "namespace" "Atoms" is used everywhere. Currently we are using the "namespace" "Unity Atoms" for all Atoms.

AdamRamberg · 2021-07-07T19:06:10Z

Packages/Core/Editor/Generator/Templates/PairTemplateFactory.cs

+            var typeName = type.CSharpName().Capitalize();
+            var pairTypeName = pairType.CSharpName().Capitalize();
+
+            template.className = template.className.Replace(pairTypeName, typeName) + atomName;


Appending Pair to the end does not seem right to me. The result becomes for example ByteEventInstancerPair, but the desired result is BytePairEventInstancer.

AdamRamberg · 2021-07-07T19:07:00Z

Packages/Core/Editor/Generator/Templates/PairTemplateFactory.cs

+            {
+                var fileName = template.attributes[typeof(CreateAssetMenuAttribute)][0];
+                fileName = $"{fileName.Replace(pairTypeName, typeName)} + \"{atomName}\"";
+                template.attributes[typeof(CreateAssetMenuAttribute)][0] = fileName;


Same problem as className above.

AdamRamberg · 2021-07-07T19:07:09Z

Packages/Core/Editor/Generator/Templates/PairTemplateFactory.cs

+
+                var menuName = template.attributes[typeof(CreateAssetMenuAttribute)][1];
+                menuName = $"{menuName.Replace(pairTypeName, typeName)} + \" {atomName}\"";
+                template.attributes[typeof(CreateAssetMenuAttribute)][1] = menuName;


Same problem as className above.

AdamRamberg · 2021-07-07T19:07:15Z

Packages/Core/Editor/Generator/Templates/PairTemplateFactory.cs

+            {
+                var menuName = template.attributes[typeof(AddComponentMenu)][0];
+                menuName = $"{menuName.Replace(pairTypeName, typeName)} + \" {atomName}\"";
+                template.attributes[typeof(AddComponentMenu)][0] = menuName;


Same problem as className above.

AdamRamberg · 2021-07-08T06:29:31Z

Packages/Core/Editor/Generator/Generator.cs

-            var currentIndex = 0;
-            var countNamespaces = new Dictionary<string, int>();
-            while (true)
+            var assetPaths = new string[solvers.Values.Count];


If we generate a set of Atoms of one type, and then change the type and regenerate the old Atoms are still there, but unreferenced by the the generator and the generator creates a new set of Atoms. In my mind we should delete the old Atoms of the old type and then create the new ones. Maybe we should show a prompt before regenerating as well, where one could choose to delete or to keep the Atoms (unreferenced by any generator)?

Unfortunately, since the MonoScript references will have to be removed because they are broken, I can't implement this feature.

CaseyHofland added 4 commits June 9, 2021 12:01

Remove Generator Window and Regenerate All Atoms

fd1f5c1

Deprecate old generation methods.

99a37b9

This was linked to issues Jun 9, 2021

[FEATURE] Utilize ScriptableObject AtomGenerator in Packages #235

Open

Create / autogenerate Unity Atoms specific script templates #61

Open

CaseyHofland removed a link to an issue Jun 9, 2021

Create / autogenerate Unity Atoms specific script templates #61

Open

CaseyHofland requested review from AdamRamberg, soraphis and miikalo June 9, 2021 10:54

CaseyHofland removed a link to an issue Jun 9, 2021

[FEATURE] Utilize ScriptableObject AtomGenerator in Packages #235

Open

CaseyHofland added the enhancement New feature or request label Jun 9, 2021

CaseyHofland added this to the v5.0 milestone Jun 9, 2021

CaseyHofland self-assigned this Jun 9, 2021

CaseyHofland added 4 commits June 12, 2021 01:21

Deprecate AtomType and AtomTypes

1c9011d

The new generator system doesn't use them.

Small fixes

990e3fc

- AtomGenerator throwing an unnecessary warning. - Resolver delegate wasn't inside the UnityAtoms.Editor namespace.

miikalo approved these changes Jun 13, 2021

View reviewed changes

CaseyHofland added 2 commits June 21, 2021 16:36

Small fixes and improvements

8045a9a

- Fixed a typo in AtomRecipe. - Deprecated the type methods working with the legacy generator. - Set the default namespace of the AtomGenerator to "UnityAtoms". - Made monoscript fields of the AtomGenerator disabled for editing.

Remove default Generator method

bea4b23

Since there is no longer a standardized "default template" the generate method that did not require a Resolver has been removed.

miikalo reviewed Jun 21, 2021

View reviewed changes

Packages/Core/Editor/Generator/Generator.cs Outdated Show resolved Hide resolved

CaseyHofland added 6 commits July 1, 2021 23:53

Generalize Templates

caf7183

Templates don't rely on anything else anymore: they can be used to generate script templates all on their own by filling out their parameters and resolving them.

Fix EditorIconPostProcessor namespace failure

f6a9930

EditorIconPostProcessor wouldn't work if it was fully typed (UnityAtoms.EditorIcon).

Generator Fixes

c3eb5e4

- Is equatable check would always return false. - If a new generator is created, Script Compilation is requested to ensure templates being added through InitiliazeOnLoad.

Add Templates

af15df1

This will add all the templates to the AtomGenerator. They have feature parity with the old generator, save for summaries.

Fix startup serialization fail

37462fc

Generators didn't serialize correctly when Unity was started. Note that there wasn't any way to fix the MonoScript serialization. They have been removed from the Generators display to not cause any confusion.

miikalo self-requested a review July 4, 2021 08:14

miikalo reviewed Jul 4, 2021

View reviewed changes

AdamRamberg reviewed Jul 4, 2021

View reviewed changes

Packages/Core/Editor/Generator/Templates/VariableTemplateFactory.cs Outdated Show resolved Hide resolved

CaseyHofland added 3 commits July 6, 2021 11:32

Remove obsolete generator stuff

a675035

AdamRamberg requested changes Jul 7, 2021

View reviewed changes

AdamRamberg reviewed Jul 8, 2021

View reviewed changes

Generator rewrite #281

Are you sure you want to change the base?

Generator rewrite #281

Conversation

CaseyHofland commented Jun 9, 2021 • edited

The generator has been rewritten to work with types.

The generator accepts a delegate of type Resolver.

Resolvers are superuperduper easy to extend.

The AtomGenerator options are dynamic and extendable.

CaseyHofland commented Jun 9, 2021

CaseyHofland commented Jun 9, 2021

CaseyHofland commented Jun 9, 2021

CaseyHofland commented Jun 11, 2021

CaseyHofland commented Jun 11, 2021 • edited

miikalo commented Jun 12, 2021 • edited

miikalo commented Jun 12, 2021

CaseyHofland commented Jun 12, 2021

CaseyHofland commented Jun 12, 2021 • edited

miikalo left a comment

Choose a reason for hiding this comment

CaseyHofland commented Jun 21, 2021 • edited

miikalo commented Jun 21, 2021

miikalo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CaseyHofland commented Jul 6, 2021

AdamRamberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CaseyHofland commented Jun 9, 2021 •

edited

CaseyHofland commented Jun 11, 2021 •

edited

miikalo commented Jun 12, 2021 •

edited

CaseyHofland commented Jun 12, 2021 •

edited

CaseyHofland commented Jun 21, 2021 •

edited