From f971cd9acae040d5057a129cd5f2300a3e47bee9 Mon Sep 17 00:00:00 2001 From: Micah Date: Mon, 6 May 2024 15:36:23 -0700 Subject: [PATCH 1/2] Link Humanoids last in `PRNT` chunks --- rbx_binary/Cargo.toml | 1 + rbx_binary/src/serializer/mod.rs | 9 ++- rbx_binary/src/serializer/state.rs | 53 +++++++++++------- rbx_binary/src/tests/serializer.rs | 19 +++++++ ...sts__serializer__parent_humanoid_last.snap | 55 +++++++++++++++++++ 5 files changed, 115 insertions(+), 22 deletions(-) create mode 100644 rbx_binary/src/tests/snapshots/rbx_binary__tests__serializer__parent_humanoid_last.snap diff --git a/rbx_binary/Cargo.toml b/rbx_binary/Cargo.toml index 9e1c3096..912a1a73 100644 --- a/rbx_binary/Cargo.toml +++ b/rbx_binary/Cargo.toml @@ -23,6 +23,7 @@ lz4 = "1.23.3" thiserror = "1.0.31" serde = { version = "1.0.137", features = ["derive"], optional = true } profiling = "1.0.6" +lazy_static = "1.4.0" [dev-dependencies] criterion = "0.3.5" diff --git a/rbx_binary/src/serializer/mod.rs b/rbx_binary/src/serializer/mod.rs index 7a2ea0e3..069577d0 100644 --- a/rbx_binary/src/serializer/mod.rs +++ b/rbx_binary/src/serializer/mod.rs @@ -1,7 +1,7 @@ mod error; mod state; -use std::io::Write; +use std::{collections::HashSet, io::Write}; use rbx_dom_weak::{types::Ref, WeakDom}; use rbx_reflection::ReflectionDatabase; @@ -10,6 +10,13 @@ use self::state::SerializerState; pub use self::error::Error; +lazy_static::lazy_static! { + /// A list of classes that should be parented last in files to prevent them + /// potentially loading before their descendants do, which can cause + /// problems like reported in https://github.com/rojo-rbx/rbx-dom/issues/406. + pub static ref LOWERED_CLASSES: HashSet<&'static str> = ["Humanoid"].into(); +} + /// A configurable serializer for Roblox binary models and places. /// /// ## Example diff --git a/rbx_binary/src/serializer/state.rs b/rbx_binary/src/serializer/state.rs index 5a70dd63..9d9a378c 100644 --- a/rbx_binary/src/serializer/state.rs +++ b/rbx_binary/src/serializer/state.rs @@ -31,7 +31,7 @@ use crate::{ Serializer, }; -use super::error::InnerError; +use super::{error::InnerError, LOWERED_CLASSES}; static FILE_FOOTER: &[u8] = b""; @@ -1246,29 +1246,40 @@ impl<'dom, 'db, W: Write> SerializerState<'dom, 'db, W> { chunk.write_u8(0)?; // PRNT version 0 chunk.write_le_u32(self.relevant_instances.len() as u32)?; - let object_referents = self - .relevant_instances - .iter() - .map(|id| self.id_to_referent[id]); - - let parent_referents = self.relevant_instances.iter().map(|id| { - let instance = self.dom.get_by_ref(*id).unwrap(); - - // If there's no parent set OR our parent is not one of the - // instances we're serializing, we use -1 to represent a null - // parent. - if instance.parent().is_some() { - self.id_to_referent - .get(&instance.parent()) - .cloned() - .unwrap_or(-1) + // We want certain classes to be parented after everything else to avoid + // problems with + let mut delayed = Vec::new(); + let mut object_referents = Vec::with_capacity(self.relevant_instances.len()); + let mut parent_referents = Vec::with_capacity(self.relevant_instances.len()); + + for referent in &self.relevant_instances { + let item = self.dom.get_by_ref(*referent).unwrap(); + if LOWERED_CLASSES.contains(item.class.as_str()) { + delayed.push(*referent) } else { - -1 + object_referents.push(self.id_to_referent[referent]); + let parent = item.parent(); + if parent.is_some() { + parent_referents.push(self.id_to_referent.get(&parent).copied().unwrap_or(-1)); + } else { + parent_referents.push(-1); + } } - }); + } + + for referent in delayed { + let item = self.dom.get_by_ref(referent).unwrap(); + object_referents.push(self.id_to_referent[&referent]); + let parent = item.parent(); + if parent.is_some() { + parent_referents.push(self.id_to_referent.get(&parent).copied().unwrap_or(-1)); + } else { + parent_referents.push(-1); + } + } - chunk.write_referent_array(object_referents)?; - chunk.write_referent_array(parent_referents)?; + chunk.write_referent_array(object_referents.into_iter())?; + chunk.write_referent_array(parent_referents.into_iter())?; chunk.dump(&mut self.output)?; diff --git a/rbx_binary/src/tests/serializer.rs b/rbx_binary/src/tests/serializer.rs index 4c5f24b0..15e7d2a3 100644 --- a/rbx_binary/src/tests/serializer.rs +++ b/rbx_binary/src/tests/serializer.rs @@ -193,3 +193,22 @@ fn default_shared_string() { let decoded = DecodedModel::from_reader(buf.as_slice()); insta::assert_yaml_snapshot!(decoded); } + +/// Test to assure that Humanoid links are generated last in the PRNT chunk. +#[test] +fn parent_humanoid_last() { + let mut tree = WeakDom::new(InstanceBuilder::new("Model")); + let humanoid = InstanceBuilder::new("Humanoid"); + let humanoid_ref = tree.insert(Ref::none(), humanoid); + let descendant = InstanceBuilder::new("Motor6D"); + let desc_ref = tree.insert(Ref::none(), descendant); + + tree.transfer_within(humanoid_ref, tree.root_ref()); + tree.transfer_within(desc_ref, humanoid_ref); + + let mut buf = Vec::new(); + to_writer(&mut buf, &tree, &[tree.root_ref()]).unwrap(); + + let decoded = DecodedModel::from_reader(buf.as_slice()); + insta::assert_yaml_snapshot!(decoded); +} diff --git a/rbx_binary/src/tests/snapshots/rbx_binary__tests__serializer__parent_humanoid_last.snap b/rbx_binary/src/tests/snapshots/rbx_binary__tests__serializer__parent_humanoid_last.snap new file mode 100644 index 00000000..b65d9a0a --- /dev/null +++ b/rbx_binary/src/tests/snapshots/rbx_binary__tests__serializer__parent_humanoid_last.snap @@ -0,0 +1,55 @@ +--- +source: rbx_binary/src/tests/serializer.rs +assertion_line: 213 +expression: decoded +--- +num_types: 3 +num_instances: 3 +chunks: + - Inst: + type_id: 1 + type_name: Humanoid + object_format: 0 + referents: + - 1 + - Inst: + type_id: 0 + type_name: Model + object_format: 0 + referents: + - 0 + - Inst: + type_id: 2 + type_name: Motor6D + object_format: 0 + referents: + - 2 + - Prop: + type_id: 1 + prop_name: Name + prop_type: String + values: + - Humanoid + - Prop: + type_id: 0 + prop_name: Name + prop_type: String + values: + - Model + - Prop: + type_id: 2 + prop_name: Name + prop_type: String + values: + - Motor6D + - Prnt: + version: 0 + links: + - - 0 + - -1 + - - 2 + - 1 + - - 1 + - 0 + - End + From f81dc3afbeb3efbb56440bf4270c514eebac5d8a Mon Sep 17 00:00:00 2001 From: Micah Date: Mon, 6 May 2024 15:52:22 -0700 Subject: [PATCH 2/2] Update Changelog --- rbx_binary/CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/rbx_binary/CHANGELOG.md b/rbx_binary/CHANGELOG.md index f0c7de05..203558e5 100644 --- a/rbx_binary/CHANGELOG.md +++ b/rbx_binary/CHANGELOG.md @@ -2,6 +2,10 @@ ## Unreleased +* Within `PRNT` chunks, `Humanoid` instances are now linked last to avoid potential load order problems. ([#411]) + +[#411]: https://github.com/rojo-rbx/rbx-dom/pull/411 + ## 0.7.4 (2024-01-16) * Add the ability to specify a `ReflectionDatabase` to use for serializing and deserializing. This takes the form of `Deserializer::reflection_database` and `Serializer::reflection_database`. ([#375])