-
Notifications
You must be signed in to change notification settings - Fork 48
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
base: master
Are you sure you want to change the base?
Unity compatibility adjustments #68
Conversation
Thanks that looks great! :) |
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. |
Also, although this is a rare case, it may be better to use 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.) |
This is a good point, this should probably not be using I've pushed up a fix commit that adds a constructor param for
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?
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 Serialization 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. 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 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
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.
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 |
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.
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. 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.)
I don't have Shapes, so I can only guess from information such as documents, but
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. |
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.
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.
You're kind of missing the point. If a system has requirements or constraints that make it attractive to be implemented as a
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 |
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. (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. )
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.) 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 |
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
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).
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.
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!
Alternate method names like
@genaray is this a reasonable change to you? |
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.) |
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. |
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. |
* 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.
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.
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 |
* 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.
8f631dd
to
2500817
Compare
How sure are we about that crash stuff? Im pretty sure that a monobehaviour can have a e.g. |
This is an example MB with an overload of
Rider itself will warn that the method overload is wrong for Unity. On Unity
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. |
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:
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.UnityPublish
into a folder in Unity. This will cause compiler errors temporarily due to the source generators.SourceGenerators
and make sure the following settings are configured.RoslynAnalyzers
label to all of them.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
andScriptableObject
types.Update()
is a special method in Unity for anything derived from MonoBehavior or ScriptableObject, even if it includes parameters. Attempting to use overloads ofUpdate()
on these types will result in Unity crashing, which prevents developers from being able to implementISystem<T>
on these types.This PR changes these methods to instead use
Execute
in place ofUpdate
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 createScriptableObject
,MonoBehavior
, and even third-partyMonoBehavior
derived systems and does not result in Unity Editor crashes.ScriptableObject
Examples of SystemsMonoBehavior
Examples of Systems@genaray The only issue with this change from
Update
toExecute
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 useExecute
overUpdate
if something like aUnity
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?