Make implementation of OptionalCoordinateFrame generic

[Dekkonot]

Currently, we treat OptionalCoordinateFrame as its own discrete type. Due to some implementation details that I've had explained to me by a Roblox engineer, I now know this is a mistake and we should instead treat this type as Option<CFrame>.

These implementation details can be summarized in two parts:

First an foremost, Roblox has handling for C++'s optional<T> in their reflection layer. This is the motivation for this issue.

Secondly, Roblox does not use optional<T> for optional implementations of CFrame because it would allocate the full size of CFrame plus the extra memory from optional. Instead they use a custom optional type (presumably named OptionalCoordinateFrame) that instead is a pointer and a boolean.

Given their memory concern does not apply to us (we have Box<T> and are also not a game engine), it seems obvious we should generalize our solution in case it ever becomes relevant again.

This will require adjustment for Rojo but should be an otherwise simple change, as we can cast OptionalCoordinateFrame directly to Optiona<CFrame> if need be.

[LPGhatguy]

We don't have a type called OptionalCoordinateFrame currently. There is a Variant item with that name, but it holds an Option<CFrame>.

What other work would we want to do in order to generalize our support? Making a member of Variant that holds a Box<Option<Variant>> opens up a lot of funny nested representations that we might not want to deal with.

[LPGhatguy]

It might be worth generalizing the binary format handling of optional types at some point. Looking back on this, I can see how the format can support any arbitrary optional type using this encoding. Hopefully that will be an easy change if/when Roblox decides to leverage that support.

[Dekkonot]

What other work would we want to do in order to generalize our support? Making a member of Variant that holds a Box<Option<Variant>> opens up a lot of funny nested representations that we might not want to deal with.

I think we'd end up with a member of Variant that held Option<Box<Variant>> over putting the entire Option in a Box but the point still stands; it'd be a pain at the moment.

Having nested variants like that complicates our current approach to (de)serialization in the binary format, to the point where we'd have to refactor a large portion of rbx-binary. It wouldn't be too invasive of a change, but it's a lot of work regardless.

If we ever get another optional type, it'd have to happen quickly but right now we can put it off and consider the consequences for various courses. It's by no means urgent, but I do believe our current approach is wrong and having a plan to fix it won't hurt us.

My proposal would be to complete the following, but not necessarily all at once.

• Figure out how this would impact upstream projects since they're slower to change
• Update the description of the type in the binary spec file
• Spin the (de)serialization of types in rbx_binary into dedicated functions so we can nest Variants like this
• Change Variant::OptionalCoordinateFrame into a Variant::Optional and update (de)serialization accordingly in rbx_xml and rbx_binary
• ???
• We support Optional

My main concern would be us getting ahead of ourselves and putting in work for nothing because it's possible Roblox won't use the same type ID if they add more Optional types in the future...

[kennethloeffler]

I recall having a discussion on Discord about generalizing optional types around the time of implementing optional CFrame. Given that there was (and still is) only one of this sort of type, I remember thinking that it was a better use of time to implement and ship the damn thing instead of blocking it behind a large refactor that provided no real value to users, even if it would be more correct. I wouldn't call it a mistake.

This is still a good idea and it probably ought to be done sooner rather than later (it's medium priority in my mind) - god knows I don't want Rojo to fall over whenever Roblox adds another optional type! 😓

Changing the spec document to describe optional generally is a great first step. I don't think it's very likely that future optionals will have a different type ID, but I suppose we can't really know until they exist. Maybe the move is to wait for one, quickly patch rbx_binary so we don't choke (assuming it uses the same type ID), and only then perform this refactor?

[Dekkonot]

I remember thinking that it was a better use of time to implement and ship the damn thing instead of blocking it behind a large refactor that provided no real value to users, even if it would be more correct. I wouldn't call it a mistake.

It was definitely the right call to put out a patch quickly since it impacted everyone to have it be unsupported. I used mistake here when I meant to say "incorrect"; it wasn't a mistake to implement it this way since it was quick and what we needed, but we should definitely do it right now that we have the opportunity.

Maybe the move is to wait for one, quickly patch rbx_binary so we don't choke (assuming it uses the same type ID), and only then perform this refactor?

This is the sort of scenario I'm hoping to avoid by opening a discussion on it now. We have a general idea of what we'd need to do, and doing it before we're forced would be ideal. Worst case, we have generalized support for no reason, which isn't the end of the world.

I consider this to be a low priority because it hasn't come up in the last few years, but the discussion should be had while we can rather than later. The spec file being updated will come Soon:tm:.