Comments (8)
I was in fact just being a fool, you'll have to forgive me. After giving it a third thought, yeah, we probably don't need to worry about this for now. It is a problem in a vacuum but it's not something that's directly possible right now unless someone specifically includes e.g. both Color
and BrickColor
in a model using something like Lune.
That said, I think we probably should still think about it when proxy properties come, since it makes sense to.
from rbx-dom.
Isn't this only a problem when the same file contains both the old and the new property (which can only occur with a crafted file)?
from rbx-dom.
I'm not sure if it's even a real issue, but you mentioned in #328 that it might cause BrickColor
to serialize and if that's the case, it's not out of the question that this could be a problem for us later. I'm not sure it's worth worrying about too much right now but it's something we should be aware of in case we add a migration where it could be a problem.
from rbx-dom.
Sorry, I probably wasn't clear enough. It is possible to create this kind of inconsistency right now:
fn main() {
let properties = [
("brickColor", Variant::BrickColor(BrickColor::DarkIndigo)),
("BrickColor", Variant::BrickColor(BrickColor::BrightRed)),
("Color", Variant::Color3(Color3::new(0.0, 0.0, 0.0))),
];
let dom = WeakDom::new(InstanceBuilder::new("ROOT").with_children([
InstanceBuilder::new("Part").with_properties(properties.clone()),
InstanceBuilder::new("Part").with_properties(properties.clone()),
InstanceBuilder::new("Part").with_properties(properties.clone()),
]));
let output = BufWriter::new(File::create("./output.rbxm").unwrap());
let _ = to_writer(output, &dom, dom.root().children());
}
And some rbx-util output of the result:
---
num_types: 1
num_instances: 3
chunks:
- Inst:
type_id: 0
type_name: Part
object_format: 0
referents:
- 0
- 1
- 2
- Prop:
type_id: 0
prop_name: BrickColor
prop_type: BrickColor
values:
- 21
- 21
- 21
- Prop:
type_id: 0
prop_name: Color3uint8
prop_type: Color3uint8
values:
- - 0
- 0
- 0
- - 0
- 0
- 0
- - 0
- 0
- 0
- Prop:
type_id: 0
prop_name: Name
prop_type: String
values:
- Part
- Part
- Part
- Prop:
type_id: 0
prop_name: brickColor
prop_type: BrickColor
values:
- 308
- 308
- 308
- Prnt:
version: 0
links:
- - 0
- -1
- - 1
- -1
- - 2
- -1
- End
When this file is loaded in Roblox Studio, the parts end up with the color in the brickColor
chunk, which is indeed the last chunk here. When rbx-dom reads this file, brickColor
and BrickColor
are migrated, and again, the final value depends on ordering.
After some testing, our assumption made with property migrations is wrong. Rather than Roblox always choosing the newer of the two properties to load, they instead seem just take load whichever property is later in the file. This is simpler but it means that the way we implement migrations is incorrect and it's only worked because we've gotten lucky.
This problem is only possible by intentionally inserting these properties into the dom after deserialization, so I don't really see it as a fault in property migration (which only happens during deserialization).
The easiest option for solving this is just to not serialize properties we have a migration for. This isn't good for projects like Lune however, so we'd need a more robust solution. To me, it feels like #277 is a valid solution here [...]
I agree with all this - migrated properties should not be serialized, it's just a question of when we should change it. The situation is probably better now than it was before (when using the BrickColor
property would just cause errors), but if a user manages to create these inconsistencies, they're going to be pretty confused.
I think I'll start building a prototype for proxy properties this week - it's looking pretty important for correctness!
from rbx-dom.
This problem is only possible by intentionally inserting these properties into the dom after deserialization, so I don't really see it as a fault in property migration (which only happens during deserialization).
I meant that this is a flaw in our reasoning for property migrations: one of the core assumptions we made was that Roblox would always prefer newer properties over older ones. That turns out to be wrong.
It doesn't really impact property migrations as they stand right now, so they don't need to change. It does need to be addressed though, which is the point of this issue!
a user manages to create these inconsistencies, they're going to be pretty confused
I'm envisioning a scenario where someone merges models together in Lune and then they end up with the wrong color for all of their parts. The model would have to be catastrophically old (I think the camelCase properties might even predate the binary format) but if we added more migrations, this could happen with something else if we got unlucky with property names.
from rbx-dom.
one of the core assumptions we made was that Roblox would always prefer newer properties over older ones
I just don't see how migrations rest on this assumption... corecii did mention this (and it is wrong), but it doesn't seem particularly relevant to how migrations function
I'm envisioning a scenario where someone merges models together in Lune and then they end up with the wrong color for all of their parts.
This is exactly the kind of scenario that migrations prevent. Migrated properties will never exist in the dom simply as a result of a deserialization operation, because there's no way to deserialize without doing migrations, and migrations will never insert the old property. For the behavior described in this issue to occur, migrated properties must be intentionally inserted and serialized
from rbx-dom.
Maybe as a stopgap, we could also perform migrations during serialization?
from rbx-dom.
I'm thinking we should prevent serialization of migrated properties before our next release. This will more closely match our old behavior, totally prevent the inconsistencies demonstrated in this issue, and prevent any extraneous changes in results from rojo build
or Lune's serializeModel
/serializePlace
. We'll fail Lune/Rojo users who want to use properties like BrickColor
and IgnoreGuiInset
, but we can't help them without more sophisticated handling of proxy properties anyway, and migrated properties will still be canonical (albeit unserializable), solving the problem in #319.
This also has me thinking... we should probably nail down some definitions for the different terms we use in reflection like "canonical," "migrated," "alias," etc.
from rbx-dom.
Related Issues (20)
- Concretely define and use terms like `alias`, `canonical`, etc.
- Support more than one root-level Instance in rbx_dom_weak
- rbx_reflector is inconsistent with storing default values for properties HOT 1
- rbx_reflector does not read Studio saving the default place file correctly sometimes
- Releases should be more automated HOT 1
- rbx_dom_lua should be a Wally package HOT 1
- Memory leak in SharedString HOT 10
- `TerrainMaterials` should implement `FromStr`
- Support for `SecurityCapabilities` tag in XML files HOT 1
- Add support for `Enum`-typed attributes HOT 2
- rbx-dom doesn't care about select properties important for correctness and least surprise HOT 7
- `Model.WorldPivotData` needs a custom property setter/getter HOT 2
- Failed to deserialize Configuration.AttributesSerialize HOT 1
- Property write failure warning should include the instance's full name
- Humanoid instances should be serialized last in files
- Serve still incorrectly changes WorldPivotData HOT 1
- Defining the Archivable property on an instance causes binary serializer to write false for Archivable properties on all other instances of the same class HOT 4
- `WriteUnknown` behavior in rbx_xml does not respect serialization of known properties
- Small Inconsistency with float & double in Attributes encoding HOT 2
- Documenting the CollisionGroupData property format
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from rbx-dom.