Open kennethloeffler opened 4 weeks ago
We probably don't want to serialize Archivable to begin with, but I don't want to mark properties unnecessarily as non-serializable, so 2 is the best option IMO.
I think the idea of Roblox deserializing Instance.archivable
is really funny though. Under what circumstances would that even come up, ha ha?
After giving this a shot, I've ran into a minor hiccup: the way we handle default properties is per-class right now. This means that in order to override a default for an abstract class like Instance
, we'd need to override it for every class that inherits from it. That's a much bigger change than I was hoping for due to the nature of Instance
.
I don't really have any good ideas to fix this. I have no problem cascading patches down to every inherited class, but it would mean we'd need to be careful with new patches to Instance
's defaults since every one would result in a rather large addition to the database.
It is a little weird how we that information duplicated... but maybe instead of adding the default value patches to all subclasses, we could change the way rbx_binary's serializer finds default properties? e.g., replace this:
with a call to a function like:
fn find_default_value(
database: &'db ReflectionDatabase,
mut class: &'db ClassDescriptor<'db>,
canonical_property_name: &str,
) -> Option<&'db Variant> {
loop {
match class.default_properties.get(canonical_property_name) {
None => {
class = database
.classes
.get(class.superclass.as_deref()?)
.expect("superclass that is Some should exist in reflection database")
}
default_value => return default_value,
}
}
}
We wouldn't have to move any existing defaults if we did this (although it's probably a good idea to deduplicate all the defaults in the future), but it would work with the obvious implementation of property default value patches
Downside: more overhead for inherited properties (especially if we eventually do move default property values into their respective classes), since this does multiple hashmap lookups for each inherited property
Minimal repro:
Output of
rbx-util view-binary weird-archivable.rbxm
:There are several things aligning that cause this and make it problematic:
Instance.Archivable
. This is because Roblox Studio never writes this property, meaning rbx-reflector cannot discover its default value.Instance.Archivable
, the binary serializer writes the boolean fallback default (which isfalse
) for instances that are missing the Archivable property.Instance.Archivable
asInstance.archivable
.Instance.Archivable
, but does loadInstance.archivable
. This means all the instances withInstance.archivable == false
will in fact be loaded that way in Roblox Studio, and thus will not be saved when publishing to Roblox or when playtesting.I thought of a few different ways we could solve this:
Instance.Archivable
asInstance.archivable
. This would still exhibit similar behavior, but since Roblox Studio does not loadInstance.Archivable
it wouldn't cause problems. The drawbacks are that it would be impossible to serialize an instance that has Archivable == false and have Roblox Studio load the property, and it would still be possible to defineInstance.archivable
and run into the same problematic behavior. Not a great option.Instance.Archivable
. No drawbacks that I can see, besides the burdens of implementation and maintenance.It seems like the only way that totally avoids breakage is 2.