From d196c5091cb5df6a83bef74b668a0f6dcfe35000 Mon Sep 17 00:00:00 2001 From: Max Date: Wed, 3 Aug 2022 19:07:06 -0500 Subject: [PATCH] Simplify usage of attributes. (#574) * Support implicit values for primitive attributes This commit adds support for strings, numbers, and booleans to be implicitly typed in attribute maps, reducing the redundancy of needing to specify their types. I also quietly adjusted one of the tests to use a more stable class/property pair. Since SourceAssetId is locked to Roblox, it could potentially disappear at any time. * Apply formatting. * Address feedback * Backwards compatible format usage. * Axe UnresolvedValueMap in favor of $attributes Attributes can be defined directly on instances, with support for unambiguous types. * Adjust test. * to_string() -> into() * Made attribute test more concise. * small cleanup * Update src/resolution.rs * Update src/resolution.rs * Update src/resolution.rs * Update src/resolution.rs Co-authored-by: Lucien Greathouse --- .../end_to_end__tests__build__attributes.snap | 17 +++- .../attributes/default.project.json | 80 ++++++++++++++++++- src/project.rs | 7 ++ src/resolution.rs | 36 ++++++++- src/snapshot_middleware/json_model.rs | 15 ++++ src/snapshot_middleware/meta_file.rs | 17 ++++ src/snapshot_middleware/project.rs | 18 +++++ 7 files changed, 184 insertions(+), 6 deletions(-) diff --git a/rojo-test/build-test-snapshots/end_to_end__tests__build__attributes.snap b/rojo-test/build-test-snapshots/end_to_end__tests__build__attributes.snap index d78f8101..04296e4e 100644 --- a/rojo-test/build-test-snapshots/end_to_end__tests__build__attributes.snap +++ b/rojo-test/build-test-snapshots/end_to_end__tests__build__attributes.snap @@ -1,6 +1,5 @@ --- source: tests/tests/build.rs -assertion_line: 99 expression: contents --- @@ -11,12 +10,24 @@ expression: contents Explicit - AgAAAAUAAABIZWxsbwIFAAAAV29ybGQGAAAAVmVjdG9yEQAAgD8AAABAAABAQA== + DQAAAAQAAABCb29sAwEKAAAAQnJpY2tDb2xvcg4BAAAABgAAAENvbG9yMw8AAAAAAAAAAAAAAAANAAAAQ29sb3JTZXF1ZW5jZRkCAAAAAAAAAAAAAAAAAIA/AACAPwAAgD8AAAAAAACAPwAAgD8AAIA/AACAPwcAAABGbG9hdDMyBQAAAAAHAAAARmxvYXQ2NAYAAAAAAAAAAAsAAABOdW1iZXJSYW5nZRsAAAAAAAAAAA4AAABOdW1iZXJTZXF1ZW5jZRcCAAAAAAAAAAAAAAAAAAAAAAAAAAAAgD8AAAAABAAAAFJlY3QcAAAAAAAAAAAAAAAAAAAAAAQAAABVRGltCQAAAAAAAAAABQAAAFVEaW0yCgAAAAAAAAAAAAAAAAAAAAAHAAAAVmVjdG9yMhAAAAAAAAAAAAcAAABWZWN0b3IzEQAAAAAAAAAAAAAAAA== - ImplicitAttributes + Implicit + AwAAAAQAAABCb29sAwEGAAAATnVtYmVyBgAAAAAAAOA/BgAAAFN0cmluZwIEAAAAVGVzdA== + + + + + LegacyExplicit + AgAAAAUAAABIZWxsbwIFAAAAV29ybGQGAAAAVmVjdG9yEQAAgD8AAABAAABAQA== + + + + + LegacyImplicit AgAAAAMAAABIZXkCBwAAAEdyYW5kbWEGAAAAVmVjdG9yEQAAgEAAAKBAAADAQA== diff --git a/rojo-test/build-tests/attributes/default.project.json b/rojo-test/build-tests/attributes/default.project.json index 44be8f16..b9bb2d95 100644 --- a/rojo-test/build-tests/attributes/default.project.json +++ b/rojo-test/build-tests/attributes/default.project.json @@ -3,7 +3,85 @@ "tree": { "$className": "Folder", + "Implicit": { + "$className": "Folder", + "$attributes": { + "Bool": true, + "Number": 0.5, + "String": "Test" + } + }, + "Explicit": { + "$className": "Folder", + "$attributes": { + "Bool": { + "Bool": true + }, + "Float32": { + "Float32": 0 + }, + "Float64": { + "Float64": 0 + }, + "UDim": { + "UDim": [0, 0] + }, + "UDim2": { + "UDim2": [[0, 0], [0, 0]] + }, + "BrickColor": { + "BrickColor": 1 + }, + "Color3": { + "Color3": [0, 0, 0] + }, + "Vector2": { + "Vector2": [0, 0] + }, + "Vector3": { + "Vector3": [0, 0, 0] + }, + "NumberSequence": { + "NumberSequence": { + "keypoints": [ + { + "time": 0, + "value": 0, + "envelope": 0 + }, + { + "time": 1, + "value": 0, + "envelope": 0 + } + ] + } + }, + "ColorSequence": { + "ColorSequence": { + "keypoints": [ + { + "time": 0, + "color": [1, 1, 1] + }, + { + "time": 1, + "color": [1, 1, 1] + } + ] + } + }, + "NumberRange": { + "NumberRange": [0, 0] + }, + "Rect": { + "Rect": [[0, 0], [0, 0]] + } + } + }, + + "LegacyExplicit": { "$className": "Folder", "$properties": { "Attributes": { @@ -19,7 +97,7 @@ } }, - "ImplicitAttributes": { + "LegacyImplicit": { "$className": "Folder", "$properties": { "Attributes": { diff --git a/src/project.rs b/src/project.rs index 53d52808..330cbd23 100644 --- a/src/project.rs +++ b/src/project.rs @@ -229,6 +229,13 @@ pub struct ProjectNode { )] pub properties: HashMap, + #[serde( + rename = "$attributes", + default, + skip_serializing_if = "HashMap::is_empty" + )] + pub attributes: HashMap, + /// Defines the behavior when Rojo encounters unknown instances in Roblox /// Studio during live sync. `$ignoreUnknownInstances` should be considered /// a large hammer and used with care. diff --git a/src/resolution.rs b/src/resolution.rs index 547e1729..56889145 100644 --- a/src/resolution.rs +++ b/src/resolution.rs @@ -1,6 +1,6 @@ use std::borrow::Borrow; -use anyhow::format_err; +use anyhow::{bail, format_err}; use rbx_dom_weak::types::{ Attributes, CFrame, Color3, Content, Enum, Matrix3, Tags, Variant, VariantType, Vector2, Vector3, @@ -28,6 +28,13 @@ impl UnresolvedValue { UnresolvedValue::Ambiguous(partial) => partial.resolve(class_name, prop_name), } } + + pub fn resolve_unambiguous(self) -> anyhow::Result { + match self { + UnresolvedValue::FullyQualified(full) => Ok(full), + UnresolvedValue::Ambiguous(partial) => partial.resolve_unambiguous(), + } + } } #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] @@ -148,6 +155,16 @@ impl AmbiguousValue { } } + pub fn resolve_unambiguous(self) -> anyhow::Result { + match self { + AmbiguousValue::Bool(value) => Ok(value.into()), + AmbiguousValue::Number(value) => Ok(value.into()), + AmbiguousValue::String(value) => Ok(value.into()), + + other => bail!("Cannot unambiguously resolve the value {other:?}"), + } + } + fn describe(&self) -> &'static str { match self { AmbiguousValue::Bool(_) => "a bool", @@ -218,12 +235,20 @@ mod test { unresolved.resolve(class, prop).unwrap() } + fn resolve_unambiguous(json_value: &str) -> Variant { + let unresolved: UnresolvedValue = serde_json::from_str(json_value).unwrap(); + unresolved.resolve_unambiguous().unwrap() + } + #[test] fn bools() { assert_eq!(resolve("BoolValue", "Value", "false"), Variant::Bool(false)); // Script.Disabled is inherited from BaseScript assert_eq!(resolve("Script", "Disabled", "true"), Variant::Bool(true)); + + assert_eq!(resolve_unambiguous("false"), Variant::Bool(false)); + assert_eq!(resolve_unambiguous("true"), Variant::Bool(true)); } #[test] @@ -247,6 +272,11 @@ mod test { // resolve("Folder", "Tags", "\"a\\u0000b\\u0000c\""), // Variant::BinaryString(b"a\0b\0c".to_vec().into()), // ); + + assert_eq!( + resolve_unambiguous("\"Hello world!\""), + Variant::String("Hello world!".into()), + ); } #[test] @@ -257,12 +287,14 @@ mod test { ); assert_eq!( - resolve("Folder", "SourceAssetId", "532413"), + resolve("IntValue", "Value", "532413"), Variant::Int64(532413), ); assert_eq!(resolve("Part", "Transparency", "1"), Variant::Float32(1.0)); assert_eq!(resolve("NumberValue", "Value", "1"), Variant::Float64(1.0)); + + assert_eq!(resolve_unambiguous("12.5"), Variant::Float64(12.5)); } #[test] diff --git a/src/snapshot_middleware/json_model.rs b/src/snapshot_middleware/json_model.rs index c29388c1..0f1388e4 100644 --- a/src/snapshot_middleware/json_model.rs +++ b/src/snapshot_middleware/json_model.rs @@ -2,6 +2,7 @@ use std::{borrow::Cow, collections::HashMap, path::Path, str}; use anyhow::Context; use memofs::Vfs; +use rbx_dom_weak::types::Attributes; use serde::Deserialize; use crate::{ @@ -78,6 +79,9 @@ struct JsonModel { skip_serializing_if = "HashMap::is_empty" )] properties: HashMap, + + #[serde(default = "HashMap::new", skip_serializing_if = "HashMap::is_empty")] + attributes: HashMap, } impl JsonModel { @@ -96,6 +100,17 @@ impl JsonModel { properties.insert(key, value); } + if !self.attributes.is_empty() { + let mut attributes = Attributes::new(); + + for (key, unresolved) in self.attributes { + let value = unresolved.resolve_unambiguous()?; + attributes.insert(key, value); + } + + properties.insert("Attributes".into(), attributes.into()); + } + Ok(InstanceSnapshot { snapshot_id: None, metadata: Default::default(), diff --git a/src/snapshot_middleware/meta_file.rs b/src/snapshot_middleware/meta_file.rs index 2715dbbe..abf53fba 100644 --- a/src/snapshot_middleware/meta_file.rs +++ b/src/snapshot_middleware/meta_file.rs @@ -1,6 +1,7 @@ use std::{borrow::Cow, collections::HashMap, path::PathBuf}; use anyhow::{format_err, Context}; +use rbx_dom_weak::types::Attributes; use serde::{Deserialize, Serialize}; use crate::{resolution::UnresolvedValue, snapshot::InstanceSnapshot}; @@ -78,6 +79,9 @@ pub struct DirectoryMetadata { #[serde(default, skip_serializing_if = "HashMap::is_empty")] pub properties: HashMap, + #[serde(default, skip_serializing_if = "HashMap::is_empty")] + pub attributes: HashMap, + #[serde(skip_serializing_if = "Option::is_none")] pub class_name: Option, @@ -139,6 +143,19 @@ impl DirectoryMetadata { snapshot.properties.insert(key, value); } + if !self.attributes.is_empty() { + let mut attributes = Attributes::new(); + + for (key, unresolved) in self.attributes.drain() { + let value = unresolved.resolve_unambiguous()?; + attributes.insert(key, value); + } + + snapshot + .properties + .insert("Attributes".into(), attributes.into()); + } + Ok(()) } } diff --git a/src/snapshot_middleware/project.rs b/src/snapshot_middleware/project.rs index 5650c99a..f8fa7920 100644 --- a/src/snapshot_middleware/project.rs +++ b/src/snapshot_middleware/project.rs @@ -2,6 +2,7 @@ use std::{borrow::Cow, collections::HashMap, path::Path}; use anyhow::{bail, Context}; use memofs::Vfs; +use rbx_dom_weak::types::Attributes; use rbx_reflection::ClassTag; use crate::{ @@ -231,6 +232,23 @@ pub fn snapshot_project_node( properties.insert(key.clone(), value); } + if !node.attributes.is_empty() { + let mut attributes = Attributes::new(); + + for (key, unresolved) in &node.attributes { + let value = unresolved.clone().resolve_unambiguous().with_context(|| { + format!( + "Unresolvable attribute in project at path {}", + project_path.display() + ) + })?; + + attributes.insert(key.clone(), value); + } + + properties.insert("Attributes".into(), attributes.into()); + } + // If the user specified $ignoreUnknownInstances, overwrite the existing // value. //