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.
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
Custom Entity Migration #232
Custom Entity Migration #232
Changes from 11 commits
755edc2
97dc150
87632a0
bae9b8e
8c94539
b4324dd
9618b65
59b135f
6a85ec9
9a72efa
d750a0c
2fafb2e
8f00359
3986537
aca1073
fe2784b
21d5b1c
24e55f5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
Should this be called
EntityRemapHelper
instead?EntityRemap...
because that's what it does but I get that it's only used by the migration feature at the moment
Helper...
It's a weird combination of util methods and extension methods. I get why they're together but Util and Extension tend to fit very specific patterns.
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.
Yep, I'm cool with that. Done.
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.
Actually renamed to
WorldEntityMigrationHelper
because of the changes here: #232 (comment)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.
You're not going to like me but since they're all extension methods now
WorldEntityMigrationExtension
maybe makes more sense?It's spring time and I really miss my 🚲
(seriously though, you can leave it as is. I'm just splitting hairs)
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.
Cool we discussed and it's called
EntityWorldMigrationX
for the Extension, System and Observer now.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.
Why are these wrapped in individual types vs just being statics in the
MigrationUtil
class?If they have to be wrapped to they need to each be wrapped int heir own type or could they all be put into on private class?
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.
This is just how
SharedStatic
's work. Unity itself has to do a similar scan forIComponentTypes
and we need a way to differentiate between theirIntPtr
and myIntPtr
. By wrapping in private sealed classes, I ensure my uniqueness and they ensure theirs.It's a bunch of boiler plate but that's why I relegated it to the bottom of this class hidden away.
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.
cool, weird pattern.
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.
SharedStatic and FunctionPointers for Burst in general are weird patterns I agree, but I guess I see how they need to ensure the uniqueness of a memory address without using any managed code and that's really only possible by using a bunch of compile time generated generics with unique class types.
Annoying though.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.