rbx-dom should populate default properties on instances

[kennethloeffler]

I'm aware of the following properties which can lead to annoying, surprising, or incorrect behavior:

• Terrain.ShorelinesUpgraded When this property is missing, Roblox Studio prompts the user to upgrade their terrain shorelines.
• Workspace.ExplicitAutoJoints When this property is missing, Roblox Studio generates Snap instances underneath contacting parts, depending on their surfaces.
• Model.NeedsPivotMigration When this property is missing, Roblox Studio does not apply model pivots.
• Perhaps others?

The question is whether rbx-dom should ensure these properties get set to sensible values, or if the responsibility should be pushed off to consumers. I'm leaning towards the former - it's not obvious whatsoever that these properties even exist! I think we could set these to good defaults in rbx_dom_weak.

[nezuo]

The Terrain.ShorelinesUpgraded one is especially annoying (because of the popup). I think the defaults should be implemented in rbx-dom as well.

[kennethloeffler]

A few more questions:

• Should rbx-dom populate all properties with their defaults, or only some?
• Should rbx_dom_weak populate properties?
  • This is a little hairy, because then rbx_dom_weak needs access to reflection
  • Or should the rbx_xml and rbx_binary (de)serializers populate properties instead?
  • This would be more straightforward since rbx_xml and rbx_binary already have access to reflection

[Dekkonot]

I think populating all default properties is probably a bad idea since it would massively increase the memory consumption for some Instances (I'm thinking about Part, as an example).

Having the parsers do it is weird because it means that they become opinionated instead of neutral. I'm thinking we may want a method or two in rbx_dom_weak to apply the default properties of an Instance when given a reflection database. That means the DOM would only need rbx_reflection as a dependency, which we update far less frequently than we do rbx_reflection_database and doesn't complicate things too much.

[kennethloeffler]

Having the parsers do it is weird because it means that they become opinionated instead of neutral. I'm thinking we may want a method or two in rbx_dom_weak to apply the default properties of an Instance when given a reflection database. That means the DOM would only need rbx_reflection as a dependency, which we update far less frequently than we do rbx_reflection_database and doesn't complicate things too much.

This sounds reasonable and would work alright, but I remain concerned about correctness, and making the pit of success as wide and easy to fall into as possible. Model.NeedsPivotMigration shows that some properties must be present, or else Roblox won't load the file properly. It's probably not the only property like this. The fact that rbx-dom's implementation can leave out properties is a divergence from Roblox. Populating all properties would not just be our opinion - it's Roblox's standard. Also, I think it's just a little strange for consumers to have to call a special method for everything to work, just as it's strange to have to insert obscure properties.

I think populating all default properties is probably a bad idea since it would massively increase the memory consumption for some Instances (I'm thinking about Part, as an example).

Considering that rbx-dom is predominantly used to read files saved by Roblox Studio - which does include all the properties - I'm not sure if this is an actual problem. As far as I know, it is not possible to get Roblox to save files that have missing properties, and you must use a tool like Rojo or Lune (or rbx-dom manually, of course). I guess we can measure the real impact, combine this with the previous observation, and go from there?

[Dekkonot]

I've ran into an instance while implementing Syncback for Rojo where information on 'correctness' would be useful. I'm curious what your thoughts would be on including properties like MeshPart.InitialSize and Model.ScaleFactor which are important to the actual function of these Instances?

For syncback, I'm stripping out as many properties as possible to ease diffing two DOMs, but there's no easy metric for this. Right now I'm just using "scriptable" and "not scriptable" but I would like to differentiate between "important" and "not important" too since there are a fair few unscriptable properties that are critical to being able to build a place file correctly.

[kennethloeffler]

Well, here it's looking like at least rbx-dom's serializers definitely should write all properties so they always produce correct results, but I'm not totally sure about Rojo's syncback. Based on the information in this issue, and the observations in your comment, I think excluding unscriptable properties from syncback would lead to incorrectness. Maybe the path forward there is considering all properties, while blacklisting any properties that are problematic for whatever reason?

[Dekkonot]

My concern with that strategy is that I imagine a lot of people will want to run syncback on their game and then just use the output for serving into Studio. If there's a bunch of properties that can't be scripted, it's a bad UX, especially for services like Lighting and Workspace, which have a fair few unscriptable properties. These are important if you're using the project for rojo build, but they aren't important if you're just syncing and having a bunch of sync errors obscures actual problems.

I'm also trying to avoid blacklisting or whitelisting properties because it isn't sustainable and feels out of place in Rojo. I'd ideally like rbx-dom to track the information I need for this implementation instead, even if that information is written manually.