Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option for RBX GUID referents #476

Open
ccuser44 opened this issue Nov 9, 2024 · 7 comments
Open

Add option for RBX GUID referents #476

ccuser44 opened this issue Nov 9, 2024 · 7 comments

Comments

@ccuser44
Copy link

ccuser44 commented Nov 9, 2024

Add an option for generating RBX GUID referents instead of sequential arithmetic counter based referents. Even though they are accepted they are non-standard and thus it would be nice to be able to generate fully standardized outputs, especially for production environments. Also some that may combine multiple outputs generated by Rojo may conflict due to sharing sequential intiger referents.

Maybe this is just a dumb idea and I'm wrong but I suspect non.

@kennethloeffler kennethloeffler transferred this issue from rojo-rbx/rojo Nov 9, 2024
@kennethloeffler
Copy link
Member

kennethloeffler commented Nov 9, 2024

I've transferred this issue to rbx-dom because I believe it's more relevant here than it is for Rojo.

I'm assuming you're referring to how rbx-dom's Roblox XML serializer writes referent attributes such that they contain values from 0 to N, where N is the number of instances described by the file.

Even though they are accepted they are non-standard and thus it would be nice to be able to generate fully standardized outputs, especially for production environments.

I agree with the general idea that rbx-dom's implementation should match Roblox's as closely as possible; however, the XML format is pretty crusty. It's disfavored because of its large size and processing time requirements, has only niche use cases, and will likely be soft-deprecated by Roblox in the near future (this is something I've heard from speaking to Roblox engineers about the topic, so unfortunately there's no public communication I can point you to here).

Also some that may combine multiple outputs generated by Rojo may conflict due to sharing sequential intiger referents.

This is not possible when working within the library (if you're talking about manipulating Roblox XML as text, then all bets are off anyway!). Roblox XML's referent attributes are used by rbx-dom only during serialization/deserialization of Roblox XML, and are irrelevant to the in-memory representation rbx-dom uses for instances.

See here how rbx_xml makes a new InstanceBuilder during deserialization:

let builder = InstanceBuilder::with_property_capacity(class_name, prop_capacity);

...which in turn generates a new referent for the in-memory representation for this instance:

pub fn with_property_capacity<S: Into<Ustr>>(class: S, capacity: usize) -> Self {
let class = class.into();
let name = class.to_string();
InstanceBuilder {
referent: Ref::new(),
name,
class,
properties: Vec::with_capacity(capacity),
children: Vec::new(),
}
}

...which is randomized, and totally disconnected from the referent attribute.

impl Ref {
/// Generate a new random `Ref`.
#[inline]
pub fn new() -> Self {
Ref(Some(rand::random()))
}

It's true that we could try to persist the referent attribute in some way, but this is significantly more complicated than the current approach, and doesn't impact functionality or correctness in any way. If this becomes the new default behavior, then it would likely just annoy users, leaving them wondering why all of their files just changed. It could be a non-default option, but I have a hard time seeing a compelling reason to include it.

@Dekkonot
Copy link
Member

Dekkonot commented Nov 9, 2024

Even though they are accepted they are non-standard and thus it would be nice to be able to generate fully standardized outputs, especially for production environments.

To add onto what Ken said, a major goal of rbx_dom (and by extension Rojo) is making our file serialization deterministic. That is, the same set of Instances with the same properties should always serialize in the same way. Roblox doesn't make a guarantee that this is the case, but we try very hard to ensure it. The referent field is one of the spots we make a compromise; rather than make every file different, we elect to not do that... even though it differs from how Roblox does it.

For what it's worth, I am 100% confident that this will never break. We've directly communicated with Roblox on this before and it's fine. The fact that they're GUIDs should be proof enough that the underlying contents don't matter, since it's clear they're nonce values generated at serialization time. Regardless though, we almost certainly won't be changing this.

Out of curiosity, what are you using the XML format for that you're worried about production? I strongly encourage using the binary format for anything that's in production since it's better in almost every way.

@ccuser44
Copy link
Author

It may make sense for Rojo to only generate sequential referents. However rbx dom is a general purpose Roblox file library and thus it makes sense to at least have it as an option. Feels kinda weird that Rojos usage gets such preferential treatment that even an option for GUID referents isn't sypported, even though it's more consistent.
Im not saying GUIDs should be used by default, not at all, but I'm saying there should be an option, the implementation is relatively simple.

@kennethloeffler
Copy link
Member

kennethloeffler commented Nov 11, 2024

It is not that simple when we take into account all the requirements: namely determinism, which is very important for a host of reasons. We can go further into why determinism makes more sense than indeterminism for a general-purpose library If you'd like, but to get it even close to right, we'd at least have to:

  • somehow persist the referent attribute in the in-memory DOM;
  • avoid collisions if instances deserialized multiple times from the same file are inserted into the same DOM.

It's probably not possible to avoid indeterminism in the second bullet point, since we'd have to generate a new random referent when there is a collision. The proposed option would also have to be able to handle files serialized by rbx-dom's current implementation.


While you do have valid points, and I appreciate the thought you've put into this request, we need to see stronger reasons for why this should be included. rbx-dom's primary goals are consistency and determinism, ensuring that the same set of instances always serialize in the same way. This has proven valuable for the majority of our users.

Adding an option for GUID-based referents introduces variability that directly contradicts this core goal. While the implementation might appear straightforward, it would complicate the serialization process and increase maintenance costs, especially given the niche use cases for Roblox XML.

That said, we can revisit this if you provide concrete, real-world examples where GUID-based referents in Roblox XML are necessary or beneficial. Additionally, community feedback showing broader interest in this feature would help us better gauge demand.

In the meantime, if you must use Roblox XML, we recommend using the existing sequential referents, which should suffice for most applications. If you’re encountering specific issues, it might be helpful to look at strategies for deduplication or referent reassignment. Let us know if you have further thoughts or if you’d like to discuss potential workarounds for your use case.

@ccuser44
Copy link
Author

That said, we can revisit this if you provide concrete, real-world examples where GUID-based referents in Roblox XML are necessary or beneficial. Additionally, community feedback showing broader interest in this feature would help us better gauge demand.

One great example is if you wan't to do any regex magic regarding the XML file that deals with referents. Like grepping or calculating the amount of unique instances, or how many references to an instance exist per reference.

Another is if you paste ot otherwise combine XML data unintellegently. There may be uses for some toolchains that do this and sequential referents could cause issues.

Adding an option for GUID-based referents introduces variability that directly contradicts this core goal. While the implementation might appear straightforward, it would complicate the serialization process and increase maintenance costs, especially given the niche use cases for Roblox XML.

Does it though? If it's an option that's disabled by default the I don't see the issue with contradicting with the determinism principle.

While the implementation might appear straightforward, it would complicate the serialization process and increase maintenance costs, especially given the niche use cases for Roblox XML.

Does it though. I suspect the change of implementation would be

generateSequentialId()
to
IF settings.GUIDReferent THEN generateGUID() ELSE generateSequentialId()

I could be wrong so correct if I'm wrong though.

That said, we can revisit this if you provide concrete, real-world examples where GUID-based referents in Roblox XML are necessary or beneficial. Additionally, community feedback showing broader interest in this feature would help us better gauge demand.

Well currently I have no use for this feature as I haven't even olayed Roblox for a year now and my only Roblox development is related to OSS project, some paid comissions and some peesonal projects I'm doing. However a few years ago something like this where I was toying with all kinds of stuff relating data serialisation and I was planning on making a public alternative JSON based spec for Roblox XML files. Though that project is totally dead now.

That being said, already as a principle, I support something like this as a feature as I do not see any reason not to have this as a feature. I just can't see any good reasons not to have this as an option that's disabled by default if I've understood things correctly.

@Dekkonot
Copy link
Member

One great example is if you wan't to do any regex magic regarding the XML file that deals with referents. Like grepping or calculating the amount of unique instances, or how many references to an instance exist per reference.

Another is if you paste ot otherwise combine XML data unintellegently. There may be uses for some toolchains that do this and sequential referents could cause issues.

We aren't interested in supporting use cases that don't actually parse the XML we produce like this. Just because the format is human-readable doesn't mean we anticipate people editing or using it like it's a text file.

Does it though. I suspect the change of implementation would be

generateSequentialId()
to
IF settings.GUIDReferent THEN generateGUID() ELSE generateSequentialId()

I could be wrong so correct if I'm wrong though.

You are in fact incorrect. This change would require that we heavily modify how referents are collected and used, and that doesn't even consider us potentially roundtripping GUIDs.

I'm going to leave this open for now because our opinions may change but right now I'm firmly saying this isn't planned. Sorry that this disappoints, but you are welcome to fork the project and implement it yourself if you need it.

@ccuser44
Copy link
Author

Does it though. I suspect the change of implementation would be

generateSequentialId()
to
IF settings.GUIDReferent THEN generateGUID() ELSE generateSequentialId()

I could be wrong so correct if I'm wrong though.

You are in fact incorrect. This change would require that we heavily modify how referents are collected and used, and that doesn't even consider us potentially roundtripping GUIDs.

Understandable. I would like to know tho what the afformentioned issues are out of curiosity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants