fix: Finalize array support in Quantum#3212
Conversation
|
|
||
| private bool ShouldGenerateItemIdCollection(object collection) | ||
| { | ||
| // Arrays do not support Collection Ids right now; |
There was a problem hiding this comment.
As it is a "right now" and talks about a known shortcoming of the current solution, I'd put a "TODO" there so we won't forget
There was a problem hiding this comment.
Created an issue (#3213) instead, TODOs feel a bit too easy to ignore, let me know if you feel like I should include it with the comment anyway
There was a problem hiding this comment.
No, you are right. An issue is better
|
I'll try to review it on the weekend. |
Kryptos-FR
left a comment
Kryptos-FR
left a comment
There was a problem hiding this comment.
Overall good, a few minor issues, and we should add more tests to cover the specifics.
Possible area:
- reflection: DescriptorTests for each descriptor type, test each methods from the base class and the correct behavior for it
- quantum: TestCollections add TestArrays
- asset: in Stride.Core.Quantum.test, serialize/deserialize a component with an array of content references, do a round trip
- archetype regression (will be useful for #3213): a base + derived asset using arrays. Confirm no exception during
ReconcileWithBaseNodeand no~Idemitted.
| } | ||
| else if (descriptor is DictionaryDescriptor dictionaryDescriptor) | ||
| { | ||
| if (dictionaryDescriptor.KeyType.IsInstanceOfType(index.Value) |
There was a problem hiding this comment.
We probably still need to check the type and short-circuit, otherwise it will throw in dictionaryDescriptor.
Either that, or opposite to the comment in DictionaryDescriptior return false instead of throwing if key is null.
Whatever the choice is, we need new tests to cover these cases.
There was a problem hiding this comment.
See other comment, will produce tests once we resolve this.
|
🤖 Draft PR — automatic CI is skipped to save runner minutes.
|
|
Kindly pinging @Kryptos-FR if he can reply to the discussion above - no rush, just in case you forgot ;) |
|
@Eideren I did forget ;). Looks ok to me now. |
Kryptos-FR
left a comment
Kryptos-FR
left a comment
There was a problem hiding this comment.
With a few more tests it should be good.
|
@Kryptos-FR Looks like the I don't think it makes sense to maintain these given that they are not really user-facing, nor particularly useful ? I added tests to cover the other changes. |
|
@Eideren yeah |
3 participants
PR Details
Arrays previously had limited support in the gamestudio, asset types could not be assigned to one of its slots for example. This PR fixes that.
Note that this PR does not affect collection id support for arrays; derived arrays still won't inherit appropriately as it requires rewriting some of the internal logic called through
AssetPropertyGraph.ReconcileWithBaseNodethat work with the expectation that one can remove and add to the collection.Also note that some of these changes refactor some of the reflection logic around collection types to move type-specific logic to their own implementation. Let me know if this is a pain on your side @Kryptos-FR and we'll see what I can do to help you out.
Related Issue
Fixes #2386
Types of changes
Checklist