Skip to content

Unity compatibility adjustments #68

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

jeffcampbellmakesgames
Copy link

@jeffcampbellmakesgames jeffcampbellmakesgames commented Feb 28, 2024

Hello, this PR is aimed at several different areas to make the features available in Arch.Extended more compatible with Unity and easier for developers to build and use.

I've based this PR on #67 so I'd hold off on merging it if there are any concerns/issues with that one.

Publish Profiles

I've created publish profiles for all the non-tests/sample projects with the correct settings such that a developer has a straightforward path to publish these projects and use them in Unity without compiler error issues. The steps are to:

  1. Publish each project using the provided profile. This will result in all assemblies being published to two folders underneath UnityPublish.
  • UnityPublish
    • Plugins => Contains assemblies that may need to be directly referenced.
    • SourceGenerators => Contains source generator assemblies only. These require a few special steps after being imported into Unity to use properly.
  1. Import the contents of UnityPublish into a folder in Unity. This will cause compiler errors temporarily due to the source generators.
  2. Select all assemblies underneath SourceGenerators and make sure the following settings are configured.
    • Go to Select platforms for plugin and disable Any Platform.
    • Go to Include Platforms and disable Editor and Standalone.
    • Go to Asset Labels and open the Asset Labels sub-menu (this is present in the lower-right corner of the inspector).
    • Assign a custom RoslynAnalyzers label to all of them.

image

image

  1. This should result in all compiler errors going away and all features of Arch.Extended can be used in Unity.

If useful these instructions could be added to the Wiki.

AOT.SourceGenerator

Unity's C# level and capabilities do not currently include [ModuleInitializer] and so this source generator did not work in Unity as a result as well as causing compiler errors. I've modified this to conditionally use the Unity equivalent [RuntimeInitializeOnLoadMethod] instead when this source generator is used in Unity projects.

I also added a small QOL debug change to sort the component types by name; this makes it easier to visually inspect the generated code in the case of any issues.

Systems

This is a helpful feature that almost seems like it could be a default for Arch, but has one issue that currently makes that impossible to use in Unity with MonoBehavior and ScriptableObject types.

Update() is a special method in Unity for anything derived from MonoBehavior or ScriptableObject, even if it includes parameters. Attempting to use overloads of Update() on these types will result in Unity crashing, which prevents developers from being able to implement ISystem<T> on these types.

This PR changes these methods to instead use Execute in place of Update which fixes this issue and updates the sample project as a result. This enabled me as a developer to make a few Unity-centric base classes for systems so that I could create ScriptableObject, MonoBehavior, and even third-party MonoBehavior derived systems and does not result in Unity Editor crashes.

ScriptableObject Examples of Systems

image

MonoBehavior Examples of Systems
image

@genaray The only issue with this change from Update to Execute is that it would be a breaking API change and perhaps not necessarily one we'd want to force on everybody for the sake of just Unity developers. I could also instead rework this to conditionally use Execute over Update if something like a Unity constant were added to both System projects, but then this would result in the locally included sample project to fail to compile when a Unity developer locally made those changes. Any thoughts on how you would like to accept changes like these that aim for compatibility with specific engines?

@genaray
Copy link
Owner

genaray commented Mar 3, 2024

Thanks that looks great! :)
Just one small question, is the .idea folder required? Normally it is not recommended to include IDE specific directorys.

@nuskey8
Copy link

nuskey8 commented Mar 3, 2024

This is great PR, but as a developer using Arch in Unity, there are a few things I don't agree with.

First, the RuntimeInitializeOnLoad attribute used in the AOT Source Generator works by default with RuntimeInitializeLoadType.AfterSceneLoad. Perhaps this should be done earlier (e.g. RuntimeInitializeLoadType.SubsystemRegistration)?

Also, in my opinion ISystem should not be implemented by MonoBehaviour or ScriptableObject, the implementation of System should be delegated to pure C# classes. I also don't think it is appropriate to make such a breaking change as to change the name of the method because of it.

@nuskey8
Copy link

nuskey8 commented Mar 3, 2024

Also, although this is a rare case, it may be better to use UnityEditor.InitializeOnLoadMethodAttribute in consideration of running it on the editor. (This can be implemented with #if UNITY_EDITOR.)

Additionally, there are better solutions such as NugetForUnity regarding how to distribute the dll. (This allows automatic dependency resolution and automatically adds RoslynAnalyzer tag to SourceGenerator.)

@jeffcampbellmakesgames
Copy link
Author

This is great PR, but as a developer using Arch in Unity, there are a few things I don't agree with.

First, the RuntimeInitializeOnLoad attribute used in the AOT Source Generator works by default with RuntimeInitializeLoadType.AfterSceneLoad. Perhaps this should be done earlier (e.g. RuntimeInitializeLoadType.SubsystemRegistration)?

This is a good point, this should probably not be using AfterSceneLoad. I'm not sure if SubsystemRegistration would be better though than AfterAssembliesLoaded; if there were any components attempting to be registered from external assemblies I'm not sure those types would be resolved. Do you have any strong information that pushes towards using one over the other?

I've pushed up a fix commit that adds a constructor param for AfterAssembliesLoaded.

Also, although this is a rare case, it may be better to use UnityEditor.InitializeOnLoadMethodAttribute in consideration of running it on the editor. (This can be implemented with #if UNITY_EDITOR.)

I'm not sure what your intent would be on making this method call editor only. Since the AOT protection is for types that il2cpp player builds that might strip out unless explicitly referenced, making this editor-only would not result in that outcome. Is there any additional context you can give for why this would need to be editor-only?

Also, in my opinion ISystem should not be implemented by MonoBehaviour or ScriptableObject, the implementation of System should be delegated to pure C# classes.

That's your opinion, but just because thats how you choose to work doesn't necessarily mean it should prevent or make it impossible for other developers to use Arch differently. There are a few good reasons why systems are easier to work with in a Unity way as ScriptableObjects or MonoBehaviors (usually for me SO's are the way to go, but there are times where MBs are impossible to avoid or very difficult).

Serialization
Sometimes it is desirable to provide a developer or designer in Unity the ability to edit fields on a system directly, at edit-time or runtime. By making it possible to build systems as SO's or MB's it's easy to do that.

image

It also comes with the added advantage that they are very modular when composing and ordering them into a World and this can be done from the Unity editor by anyone (even a designer) rather than only by a programmer in code. I can now build prototype scenes that only use a subset of the overall systems for testing a particular feature where the production scene would reference the ones only needed for the live game.

image

It's possible to do the serialization part as POCO systems where those systems are serialized directly inline on a Unity SO or MB, but then the grouping/ordering is all done from code only (less ideal).

Third-Party Plugins and/or Implementation Constraints
While I would rather not ever write my systems as MonoBehaviors due to unneeded extra baggage, sometimes it is unavoidable due to third-party plugins or other potential implementation constraints (a system needs access to APIs that are only available to MonoBehaviors).

For example, to create systems using the Shapes library that I use for part of the in-game UX/UI it was best if my systems inherited from their ImmediateModeShapeDrawer type which embeds well in their library and is derived ultimately from MonoBehaviors. Working around this constraint with a POCO class would have been much more complex and onerous.

image

I also don't think it is appropriate to make such a breaking change as to change the name of the method because of it.

That was also my thought as well (if you read the bottom of the PR, I had been looking for reasonable alternatives to a breaking API change). See the below comment I'd listed there, curious to hear if there are any alternatives that would enable both solutions outside of conditional compilation.

@genaray The only issue with this change from Update to Execute is that it would be a breaking API change and perhaps not necessarily one we'd want to force on everybody for the sake of just Unity developers. I could also instead rework this to conditionally use Execute over Update if something like a Unity constant were added to both System projects, but then this would result in the locally included sample project to fail to compile when a Unity developer locally made those changes. Any thoughts on how you would like to accept changes like these that aim for compatibility with specific engines?

Additionally, there are better solutions such as NugetForUnity regarding how to distribute the dll. (This allows automatic dependency resolution and automatically adds RoslynAnalyzer tag to SourceGenerator.)

I'm not sure what your point here is; providing an easy path for any developer to be able to publish the runtime assemblies locally for easy inclusion in a project doesn't prevent someone else from using something like NugetForUnity. For a developer like myself that is often modifying or augmenting the source for a project (perhaps even for client projects where the changes might not be PR'ed back to the repo), building the assemblies locally is the only solution to enabling that workflow. Using NugetForUnity or even Unity's package manager would not be viable options in that scenario.

I've seen several times on the Discord where users have asked how to build and use Arch or Arch.Extended projects in Unity and these publish profiles would enable them to easily do that (for the latter at least).

@nuskey8
Copy link

nuskey8 commented Mar 4, 2024

I'm not sure what your intent would be on making this method call editor only. Since the AOT protection is for types that il2cpp player builds that might strip out unless explicitly referenced, making this editor-only would not result in that outcome. Is there any additional context you can give for why this would need to be editor-only?

You're right. I was concerned that it would be registered twice in the ComponentRegistry, but by reading the implementation I realized that it was checked.

It's possible to do the serialization part as POCO systems where those systems are serialized directly inline on a Unity SO or MB, but then the grouping/ordering is all done from code only (less ideal).

In ECS, system should not maintain state in the first place. The state should be linked to the entity as a component, and the fact that system serialization is necessary is itself a code smell.
Regarding execution, instead of implementing ISystem with MonoBehaviour, it would be ideal to schedule a pure C# class System on PlayerLoop like Unity ECS.

Also, all system usage should be visible from the code. Although solutions using SO are easy, they become more complex to manage as development progresses. (I have seen projects that failed in the final stage of development as a result of designing with SO as the main focus.)

While I would rather not ever write my systems as MonoBehaviors due to unneeded extra baggage, sometimes it is unavoidable due to third-party plugins or other potential implementation constraints (a system needs access to APIs that are only available to MonoBehaviors).

I don't have Shapes, so I can only guess from information such as documents, but ImmediateModeShapeDrawer is just a wrapper for OnPreRender, and it seems impossible to trigger it at any timing. Is it necessary to forcefully implement ISystem under these constraints? It should be enough to call World.Query() whenever needed.

I'm not sure what your point here is; providing an easy path for any developer to be able to publish the runtime assemblies locally for easy inclusion in a project doesn't prevent someone else from using something like NugetForUnity. For a developer like myself that is often modifying or augmenting the source for a project (perhaps even for client projects where the changes might not be PR'ed back to the repo), building the assemblies locally is the only solution to enabling that workflow. Using NugetForUnity or even Unity's package manager would not be viable options in that scenario.

There was insufficient explanation on this point. What I'm trying to say here is that it's probably more beneficial for most users to use NugetForUnity. Of course, there are times when this PR solution is necessary. However, when describing the installation method on the wiki, it is better to include both.

@jeffcampbellmakesgames
Copy link
Author

It's possible to do the serialization part as POCO systems where those systems are serialized directly inline on a Unity SO or MB, but then the grouping/ordering is all done from code only (less ideal).

In ECS, system should not maintain state in the first place. The state should be linked to the entity as a component, and the fact that system serialization is necessary is itself a code smell.

It seems like you're conflating Unity editor serialization (being able to edit fields of an object in the editor) as runtime game state or something like save data serialization (wholly different concepts). In the example screenshot I provided below of one of my selection systems, making it an SO (or even in the example where it's inline as a POCO class) makes it possible for me to provide it what is essentially global game data or even external systems that do not need to live as an ECS-oriented entity or component.

image

Regarding execution, instead of implementing ISystem with MonoBehaviour, it would be ideal to schedule a pure C# class System on PlayerLoop like Unity ECS.

Also, all system usage should be visible from the code. Although solutions using SO are easy, they become more complex to manage as development progresses. (I have seen projects that failed in the final stage of development as a result of designing with SO as the main focus.)

With all due respect, this seems more like a personal preference or team style than a Unity-wide best practice or requirement for shipping games. We've shipped and maintain several different games/apps with heavy SO usage for context. There are many resources out there about Scriptable Architecture. While I don't agree with your sentiment that everything needs to be in code, this PR isn't trying to prevent you from doing that. It's making it more possible for developers who use a different architectural approach to be able to use Arch more easily.

While I would rather not ever write my systems as MonoBehaviors due to unneeded extra baggage, sometimes it is unavoidable due to third-party plugins or other potential implementation constraints (a system needs access to APIs that are only available to MonoBehaviors).

I don't have Shapes, so I can only guess from information such as documents, but ImmediateModeShapeDrawer is just a wrapper for OnPreRender, and it seems impossible to trigger it at any timing. Is it necessary to forcefully implement ISystem under these constraints? It should be enough to call World.Query() whenever needed.

You're kind of missing the point. If a system has requirements or constraints that make it attractive to be implemented as a MonoBehavior, this PR is aiming to make that possible whereas attempting to do that now results in the editor crashing. I'd like to try to make changes to make this more easily possible for a Unity developer short of needing to maintain a local fork with these or similar changes and am soliciting for preference/options from @genaray and others interested on how best to do that.

I'm not sure what your point here is; providing an easy path for any developer to be able to publish the runtime assemblies locally for easy inclusion in a project doesn't prevent someone else from using something like NugetForUnity. For a developer like myself that is often modifying or augmenting the source for a project (perhaps even for client projects where the changes might not be PR'ed back to the repo), building the assemblies locally is the only solution to enabling that workflow. Using NugetForUnity or even Unity's package manager would not be viable options in that scenario.

There was insufficient explanation on this point. What I'm trying to say here is that it's probably more beneficial for most users to use NugetForUnity. Of course, there are times when this PR solution is necessary. However, when describing the installation method on the wiki, it is better to include both.

That's what I'm proposing; an easier way for a developer using Rider (pretty common on macOS for Unity development) to be able to publish these assemblies locally with the right settings for them to work in Unity that can live alongside other methods, like Nuget or even a Unity Package Manager distro (I don't think there is one for Arch at the moment).

@annulusgames I definitely appreciate your interest and feedback (I've starred a few of your repos!), but wish that it was more open-minded and less reductive towards there being only one way to make games. I'm definitely cognizant that some developers might really dislike ScriptableObjects, but this PR isn't attempting to force you to use them ;)

@nuskey8
Copy link

nuskey8 commented Mar 4, 2024

It seems like you're conflating Unity editor serialization (being able to edit fields of an object in the editor) as runtime game state or something like save data serialization (wholly different concepts). In the example screenshot I provided below of one of my selection systems, making it an SO (or even in the example where it's inline as a POCO class) makes it possible for me to provide it what is essentially global game data or even external systems that do not need to live as an ECS-oriented entity or component.

Ideally, in the context of ECS, all of these should be accessed via components. For example, in Unity ECS, you construct a BlobAsset in the authoring component and pass the BlobAssetReference to the component.
However, I am skeptical that the benefits of this restriction are worth the effort, and it would be easier to use SerializeField directly. (Although this is sufficient for practical purposes, please note that strictly speaking, this is against the philosophy of ECS.)

(For those reading this, I would like to add one thing: Serialization of the editor and serialization of save data are, in concept, exactly the same. The only difference is whether it is done by the user or by Unity in the editor. )

With all due respect, this seems more like a personal preference or team style than a Unity-wide best practice or requirement for shipping games. We've shipped and maintain several different games/apps with heavy SO usage for context. There are many resources out there about Scriptable Architecture. While I don't agree with your sentiment that everything needs to be in code, this PR isn't trying to prevent you from doing that. It's making it more possible for developers who use a different architectural approach to be able to use Arch more easily.

The concept of ScriptableArchitecture is now considered a bad practice by many developers. (The argument here is a bit extreme, but I generally agree with it.)
As an alternative, my advice based on experience is to use Rx for messaging and a DI container for dependency resolution. However, if it can be maintained, it may be sufficient.

In any case, as you say, it may be in the developer's interest to be able to take multiple approaches. If you want to change the method name, I recommend OnUpdate rather than Execute. (This also matches Unity ECS System.)

@jeffcampbellmakesgames
Copy link
Author

jeffcampbellmakesgames commented Mar 4, 2024

Ideally, in the context of ECS, all of these should be accessed via components. For example, in Unity ECS, you construct a BlobAsset in the authoring component and pass the BlobAssetReference to the component.

You kind of have to do it this way with Unity's first-party ECS framework; there aren't other ways provided. Their framework is perhaps not the best to use as an example seeing how it's now considered production-ready, but doesn't have support for animation and other essential features without using third-party paid or open-source plugins.

Just to take this sound example further though, in the event I did want to have specific selection sounds per object I likely would use some kind of flyweight-like pattern where an index or enum value on a component would be used by a system to retrieve the appropriate AudioClip from an external factory. Right now at this early stage of development that's not important.

However, I am skeptical that the benefits of this restriction are worth the effort, and it would be easier to use SerializeField directly. (Although this is sufficient for practical purposes, please note that strictly speaking, this is against the philosophy of ECS.)

You're exactly right here though in that sometimes it's not beneficial for practical reasons to deconstruct every piece of data into components. It's better to not lock yourself into needing to implement everything into an ECS.

Where people get tripped up on ECS as a design pattern is thinking that using it means everything now has to be implemented in that paradigm, even features that don't benefit from it. This is kind of silly when you think about it; it limits the ability to integrate any sort of external functionality outside of the ECS in the game if it can't be boiled down to entities, components, or systems (should localization be implemented as an ECS system for example or is it better served as a non-ECS API?).

ECS is best-used as it benefits a project to be integrated into it and nothing more. It's really great for the simulation and core-game logic portion of my game, but there are other gameplay systems which maybe only have a tiny representation (like Cinemachine for camera movement/behavior).

**(For those reading this, I would like to add one thing: Serialization of the editor and serialization of save data are, in concept, exactly the same. The only difference is whether it is done by the user or by Unity in the editor. )

They use similar technologies, but thats about it. Making fields on a Unity object serializable enables developers to edit them in the Editor for development purposes. This isn't seen outside of the editor by users much in the way that private fields aren't seen outside of the class by consumers of an object's API.

The concept of ScriptableArchitecture is now considered a bad practice by many developers. (The argument here is a bit extreme, but I generally agree with it.)
As an alternative, my advice based on experience is to use Rx for messaging and a DI container for dependency resolution. However, if it can be maintained, it may be sufficient.

Again, this seems like a very black and white, binary hot-take on a technology when we live in a world of gray. I feel like I've said my piece on different development practices and this PR isn't forcing an approach on others as much as it is giving reasonable options.

I definitely agree with shades of the arguments that developer states (we use a lot of plain C# events alongside scriptable events for example), but I know other people who feel like a lot of DI containers and RX libraries are also unnecessarily complex. It's a great big melting pot of opinions out there!

In any case, as you say, it may be in the developer's interest to be able to take multiple approaches. If you want to change the method name, I recommend OnUpdate rather than Execute. (This also matches Unity ECS System.)

Alternate method names like OnUpdate is a reasonable solution to achieving the same outcome as Execute though it also is a breaking API change. If we were to follow this tract I'd be changing the system/group method names like so:

  • BeforeUpdate => OnBeforeUpdate
  • Update => OnUpdate
  • AfterUpdate => OnAfterUpdate

@genaray is this a reasonable change to you?

@nuskey8
Copy link

nuskey8 commented Mar 4, 2024

In any case, we probably shouldn't discuss design any further here. Again, I would like to emphasize my concern that changing method names is a breaking change and many projects and libraries will be affected accordingly. (In my personal opinion, it's unlikely that it would be profitable enough to make the change, but I'll leave it up to the collaborators to decide whether to merge or not.)

@genaray
Copy link
Owner

genaray commented Mar 6, 2024

What a discussion, it took me a long time to read through all this but I admire your dedication ^^ The only thing that logically worries me are the breaking changes... because arch is now used by many people and that could make some things incompatible. Especially the source generators have to be adapted.

Does Unity really crash if you add more update methods with a different signature? I can hardly imagine it, but I have never tested it. Would it be an alternative to replace the method names with flags? That would only be a breaking change for Unity users who use the source.

@nuskey8
Copy link

nuskey8 commented Mar 6, 2024

Would it be an alternative to replace the method names with flags? That would only be a breaking change for Unity users who use the source.

I don't really agree with the idea of changing method names with flags. The method name mismatch is confusing and if it is going to be changed, it should be changed in its entirety.

jzapdot added 2 commits March 6, 2024 08:47
* Modified Csharp projects (non unit-tests) to include SatelliteResourceLanguages tag. This prevents tons of extranneous Humanizer assemblies and resource files from being included in the publishing output.
* Set MsBuildVersion to use latest version; this prevents compiler errors on a default Rider solution open.
@jeffcampbellmakesgames
Copy link
Author

What a discussion, it took me a long time to read through all this but I admire your dedication ^^ The only thing that logically worries me are the breaking changes... because arch is now used by many people and that could make some things incompatible. Especially the source generators have to be adapted.

I wouldn't be afraid of breaking changes so much as making sure whatever the new method name is makes sense and doesn't change again for a similar reason. It took me about half an hour to go through and make this example change originally for both System projects, will not take long to adapt to a different name.

Does Unity really crash if you add more update methods with a different signature? I can hardly imagine it, but I have never tested it. Would it be an alternative to replace the method names with flags? That would only be a breaking change for Unity users who use the source.

There are pros and cons. Like @annulusgames mentioned it does make for an inconsistency between method names which can be confusing to document in API versus a one-time method name change that applies to everyone. It would also mean the Arch.Extended.Sample wouldn't compile when a user had set the flag to build the system projects for Unity, aka the solution would fail to build while that flag was set (all projects other than the sample would succeed in building).

jzapdot added 6 commits March 6, 2024 10:20
* Created publish profiles that create Unity-friendly versions of all assemblies. Plugins can be imported into Unity as-is while SourceGenerator assemblies require being tagged with ‘RoslynAnalyzer` to work properly.
* Modified AOT SourceGenerator to be compatible with Unity by using the `[RuntimeInitializeOnLoadMethod]` attribute over `[ModuleInitializer]` in Unity projects.
* Modified AOT file generated to have component types sorted by name (a-z).
`Update()` is a special method in Unity for anything derived from MonoBehavior or ScriptableObject, even if it includes parameters. Attempting to use overloads of `Update()` on these types will result in Unity crashing, which prevents developers from being able to implement `ISystem<T>` on these types.

This commit changes these methods to instead use `Execute` in place of `Update` which fixes this issue and updates the sample project as a result.
* Modified sample project to compile successfully by setting a target language version of 11. This is required as other projects are set to this language version and by not having an explicit version set Rider defaults the version to 10, which fails compilation.
@genaray
Copy link
Owner

genaray commented Mar 12, 2024

How sure are we about that crash stuff? Im pretty sure that a monobehaviour can have a e.g. Update(MyCustomClass) method besides the default update method.

@jeffcampbellmakesgames
Copy link
Author

How sure are we about that crash stuff? Im pretty sure that a monobehaviour can have a e.g. Update(MyCustomClass) method besides the default update method.

This is an example MB with an overload of Update.

 using System.Collections;
using System.Collections.Generic;
using UnityEngine;

public class ExampleBadMonoBehaviour : MonoBehaviour
{
	public void Update(float timeDelta)
	{

	}
}

Rider itself will warn that the method overload is wrong for Unity.

image

On Unity 2022.3.16f1 Unity will show script compiling errors.

Script error (ExampleBadMonoBehaviour): Update() can not take parameters.

image

In the test project I just ran this in it didn't cause a crash, but in the one I was using Arch in changing these to a non-Unity protected method for the Arch systems I was creating did.

Either way, this just points to it not being an ideal method name choice with Unity in mind.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants