-
Notifications
You must be signed in to change notification settings - Fork 1
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
Upgrading to Entities 1.0.16 #301
Merged
Merged
Changes from all commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
8510333
Initial work to make it compile
jkeon fdc0b19
Merge remote-tracking branch 'origin/main' into upgrade-path
jkeon 977cb20
Upgrading to 1.0
jkeon 2863240
API upgrades
jkeon 22cd3f3
Removing warnings
jkeon 32d3b8e
WIP
jkeon 185e627
Fixing warnings
jkeon a2f2925
Updating for the new Parallel Access
jkeon 9b5a720
Updating TaskJobForDefer to attempt to get Burst to work
jkeon 8d80d3c
Updated ITaskJobForDefer to be de-genericized
jkeon 2638718
De-genericizing Update and Cancellation Update jobs
jkeon c50808d
Working Burstable job types
jkeon ae9d919
Ready for PR
jkeon 78c9097
Updated README
jkeon 36a8d40
Removing code that was for a 1.2 entities upgrade but we decided to s…
jkeon 4904a7b
Adding back in the indexer
jkeon 1ee4bb0
Adding TODO reference
jkeon bf85fcf
Better checks
jkeon de00e7c
Accounting for enabled
jkeon e7de20a
Including disabled entities and components
jkeon a5324a9
Adding comments for why a function exists
jkeon 63f405b
Adding last comment
jkeon File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,16 +14,15 @@ The code is reasonably clean but documentation and examples are sparse. Feel fre | |
⚠️ We welcome PRs and bug reports but making this repo a public success is not our priority. No promises on when it will be addressed! | ||
|
||
# Dependencies | ||
- [Unity 2021.3.5+](https://unity.com/) | ||
- [Unity 2022.3.15+](https://unity.com/) | ||
- [anvil-csharp-core (main...usually 😬)](https://github.com/decline-cookies/anvil-csharp-core) | ||
- [anvil-unity-core (main...usually 😬)](https://github.com/decline-cookies/anvil-unity-core) | ||
- *DOTS Packages* | ||
- [com.unity.entities (0.50.1-preview2)](https://docs.unity3d.com/Packages/[email protected]/manual/index.html) | ||
- [com.unity.rendering.hybrid (0.50.0-preview.44)](https://docs.unity3d.com/Packages/[email protected]/manual/index.html) | ||
- [com.unity.burst (1.6.4)](https://docs.unity3d.com/Packages/[email protected]/manual/index.html) | ||
- [com.unity.collections (1.2.3)](https://docs.unity3d.com/Packages/[email protected]/manual/index.html) | ||
- [com.unity.jobs (0.50.0-preview.9)](https://docs.unity3d.com/Packages/[email protected]/manual/index.html) | ||
- [com.unity.mathematics (1.2.5)](https://docs.unity3d.com/Packages/[email protected]/manual/index.html) | ||
- [com.unity.entities (1.0.16)](https://docs.unity3d.com/Packages/[email protected]/manual/index.html) | ||
- [com.unity.entities.graphics (1.0.16)](https://docs.unity3d.com/Packages/[email protected]/manual/index.html) | ||
- [com.unity.burst (1.8.11)](https://docs.unity3d.com/Packages/[email protected]/manual/index.html) | ||
- [com.unity.collections (2.1.4)](https://docs.unity3d.com/Packages/[email protected]/manual/index.html) | ||
- [com.unity.mathematics (1.2.6)](https://docs.unity3d.com/Packages/[email protected]/manual/index.html) | ||
|
||
At this point in time, all DOTS related functionality is in this one repository. In the future, we may split the functionality into specialized repositories. Maintaining a repository per Unity DOTS package seems overly tedious at the moment. | ||
|
||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we should put the
[BurstCompile]
attribute on this or in any place we've replaced the attribute?OR
Since we're no longer using this attribute should we communicate methods/types that are/aren't burst compatible somehow? Our own attribute or maybe just comments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's chat about this.
Putting
[BurstCompile]
won't do anything so I think that's confusing to add.See: https://forum.unity.com/threads/when-where-and-why-to-put-burstcompile-with-mild-under-the-hood-explanation.1344539/ for more in-depth explanations for when to use the attributes.
A class/function etc isn't necessarily just Burst or not. It could be both depending on where you call it.
For example, if you called this
FixedListInternalExtension.IndexOf
from within a bursted job it would execute as Bursted code. Great.But in the same runtime, if you called the same function
FixedListInternalExtension.IndexOf
from the main thread in OO land, it would just execute as regular non-bursted code.The compiler would have generated two different lower level paths.
We could add in the updated test attribute of
GenerateTestsForBurstCompatibility
which replacesBurstCompatible
but that also doesn't do anything unless you run the editor commands for generating tests for your code base and running them. There is also no replacement forNotBurstCompatible
.If we added our own attributes, that'd be fine, but again they can't really do anything. There's nothing we can use them for to check or gate or validate for.
Comments could be fine, but I'm not sure if it's worthwhile since it will depend on the context that you all it.
In my mind, based on how things work and the feedback we get from the IDE, there are the following scenarios:
BurstCompile
attribute.Given this, I'm not sure we need to communicate anything to the developer since really they just need to understand where they are calling from.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I misunderstood how
[BurstCompile]
worked. That's fine.I think comments might be helpful. It's fair enough that the IDE will warn you when calling non-burstable code from a bursted context but it doesn't really help when you're writing bursted code and poking around the codebase to see what you can find. Until you type it out (or read the whole class) you won't know if a piece of existing functionality that you've found will be compatible.
Maybe that doesn't happen often enough to justify writing comments and I'm inventing work here but I feel like, while writing bursted code, I'm often asking myself "can I use this method/type?"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The counter-argument to comments is that it might be too general and the underlying code changes and whoever does that forgets to update the comment at the top of the file?
In the scenario you're describing, I feel like the effort to type it out (which is pretty fast with autocomplete) to get the Burst warning in the IDE is the same or less then opening things up and reading comments which would be more to maintain?
If we really wanted to signify to the developer that these are Burst safe things to use, I would argue that we should note that in the Class name or Method name instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's try without the comments for now and see how it goes.
When I'm devising a solution I'm not going to type out and check every method/type as I go. I'm going to collect a path or collection of types that "will work" and then go to implement.
Adding burst to all the type and method names is insane but if you're considering that I'll just drop this topic. You've backed me away from the ledge...