From c8310c4e57bcb17cffdba4dc9b7f061aa3828b95 Mon Sep 17 00:00:00 2001 From: nezuo Date: Sat, 4 May 2024 15:39:24 -0700 Subject: [PATCH 01/11] Add msgpack submodule --- .gitmodules | 3 +++ plugin/Packages/msgpack-luau | 1 + 2 files changed, 4 insertions(+) create mode 160000 plugin/Packages/msgpack-luau diff --git a/.gitmodules b/.gitmodules index 46789ed14..678237a2b 100644 --- a/.gitmodules +++ b/.gitmodules @@ -16,3 +16,6 @@ [submodule "plugin/Packages/Highlighter"] path = plugin/Packages/Highlighter url = https://github.com/boatbomber/highlighter.git +[submodule "plugin/Packages/msgpack-luau"] + path = plugin/Packages/msgpack-luau + url = https://github.com/cipharius/msgpack-luau diff --git a/plugin/Packages/msgpack-luau b/plugin/Packages/msgpack-luau new file mode 160000 index 000000000..0754b1d5a --- /dev/null +++ b/plugin/Packages/msgpack-luau @@ -0,0 +1 @@ +Subproject commit 0754b1d5a6919392fe497979c69b90a38a060c71 From 4385fcad858ba7f26c9b0adbe4cc24c9ae45be3d Mon Sep 17 00:00:00 2001 From: nezuo Date: Sat, 4 May 2024 15:39:35 -0700 Subject: [PATCH 02/11] Update tools --- aftman.toml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/aftman.toml b/aftman.toml index 5273b6d07..8de6c658a 100644 --- a/aftman.toml +++ b/aftman.toml @@ -1,5 +1,5 @@ [tools] -rojo = "rojo-rbx/rojo@7.3.0" -selene = "Kampfkarren/selene@0.26.1" -stylua = "JohnnyMorganz/stylua@0.18.2" +rojo = "rojo-rbx/rojo@7.4.1" +selene = "Kampfkarren/selene@0.27.1" +stylua = "JohnnyMorganz/stylua@0.20.0" run-in-roblox = "rojo-rbx/run-in-roblox@0.3.0" From 25a09acd7b4018a6381a82ad3d509b65bb4ce6a6 Mon Sep 17 00:00:00 2001 From: nezuo Date: Sat, 4 May 2024 16:21:24 -0700 Subject: [PATCH 03/11] Use msgpack for responses --- Cargo.lock | 9 +++++---- Cargo.toml | 1 + build.rs | 5 +++++ plugin/http/Response.lua | 8 +++++++- plugin/src/ApiContext.lua | 19 +++++++++++-------- plugin/watch-build.sh | 2 +- src/web/api.rs | 12 ++++++------ src/web/util.rs | 25 +++++++++++++++++++++++-- 8 files changed, 59 insertions(+), 22 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 67f82f52d..914ddec5e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1719,9 +1719,9 @@ dependencies = [ [[package]] name = "rmp" -version = "0.8.12" +version = "0.8.14" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7f9860a6cc38ed1da53456442089b4dfa35e7cedaa326df63017af88385e6b20" +checksum = "228ed7c16fa39782c3b3468e974aec2795e9089153cd08ee2e9aefb3613334c4" dependencies = [ "byteorder", "num-traits", @@ -1730,9 +1730,9 @@ dependencies = [ [[package]] name = "rmp-serde" -version = "1.1.2" +version = "1.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "bffea85eea980d8a74453e5d02a8d93028f3c34725de143085a844ebe953258a" +checksum = "52e599a477cf9840e92f2cde9a7189e67b42c57532749bf90aea6ec10facd4db" dependencies = [ "byteorder", "rmp", @@ -1786,6 +1786,7 @@ dependencies = [ "rbx_xml", "reqwest", "ritz", + "rmp-serde", "roblox_install", "rojo-insta-ext", "semver", diff --git a/Cargo.toml b/Cargo.toml index a35ccd942..a81422d41 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -90,6 +90,7 @@ tokio = { version = "1.36.0", features = ["rt", "rt-multi-thread"] } uuid = { version = "1.7.0", features = ["v4", "serde"] } clap = { version = "3.2.25", features = ["derive"] } profiling = "1.0.15" +rmp-serde = "1.3.0" [target.'cfg(windows)'.dependencies] winreg = "0.10.1" diff --git a/build.rs b/build.rs index 9f9a606bb..0d5f1a574 100644 --- a/build.rs +++ b/build.rs @@ -26,6 +26,11 @@ fn snapshot_from_fs_path(path: &Path) -> io::Result { continue; } + // Ignore images in msgpack-luau because they aren't UTF-8 encoded. + if file_name.ends_with(".png") { + continue; + } + let child_snapshot = snapshot_from_fs_path(&entry.path())?; children.push((file_name, child_snapshot)); } diff --git a/plugin/http/Response.lua b/plugin/http/Response.lua index ce4e1b6d2..b02ff6443 100644 --- a/plugin/http/Response.lua +++ b/plugin/http/Response.lua @@ -1,5 +1,7 @@ local HttpService = game:GetService("HttpService") +local msgpack = require(script.Parent.Parent.msgpack) + local stringTemplate = [[ Http.Response { code: %d @@ -31,4 +33,8 @@ function Response:json() return HttpService:JSONDecode(self.body) end -return Response \ No newline at end of file +function Response:msgpack() + return msgpack.decode(self.body) +end + +return Response diff --git a/plugin/src/ApiContext.lua b/plugin/src/ApiContext.lua index 68d430953..801137da9 100644 --- a/plugin/src/ApiContext.lua +++ b/plugin/src/ApiContext.lua @@ -122,7 +122,7 @@ function ApiContext:connect() return Http.get(url) :andThen(rejectFailedRequests) - :andThen(Http.Response.json) + :andThen(Http.Response.msgpack) :andThen(rejectWrongProtocolVersion) :andThen(function(body) assert(validateApiInfo(body)) @@ -140,7 +140,7 @@ end function ApiContext:read(ids) local url = ("%s/api/read/%s"):format(self.__baseUrl, table.concat(ids, ",")) - return Http.get(url):andThen(rejectFailedRequests):andThen(Http.Response.json):andThen(function(body) + return Http.get(url):andThen(rejectFailedRequests):andThen(Http.Response.msgpack):andThen(function(body) if body.sessionId ~= self.__sessionId then return Promise.reject("Server changed ID") end @@ -185,11 +185,14 @@ function ApiContext:write(patch) body = Http.jsonEncode(body) - return Http.post(url, body):andThen(rejectFailedRequests):andThen(Http.Response.json):andThen(function(responseBody) - Log.info("Write response: {:?}", responseBody) + return Http.post(url, body) + :andThen(rejectFailedRequests) + :andThen(Http.Response.msgpack) + :andThen(function(responseBody) + Log.info("Write response: {:?}", responseBody) - return responseBody - end) + return responseBody + end) end function ApiContext:retrieveMessages() @@ -214,7 +217,7 @@ function ApiContext:retrieveMessages() end) end - return sendRequest():andThen(rejectFailedRequests):andThen(Http.Response.json):andThen(function(body) + return sendRequest():andThen(rejectFailedRequests):andThen(Http.Response.msgpack):andThen(function(body) if body.sessionId ~= self.__sessionId then return Promise.reject("Server changed ID") end @@ -230,7 +233,7 @@ end function ApiContext:open(id) local url = ("%s/api/open/%s"):format(self.__baseUrl, id) - return Http.post(url, ""):andThen(rejectFailedRequests):andThen(Http.Response.json):andThen(function(body) + return Http.post(url, ""):andThen(rejectFailedRequests):andThen(Http.Response.msgpack):andThen(function(body) if body.sessionId ~= self.__sessionId then return Promise.reject("Server changed ID") end diff --git a/plugin/watch-build.sh b/plugin/watch-build.sh index 91ce6f894..7ffb9d278 100644 --- a/plugin/watch-build.sh +++ b/plugin/watch-build.sh @@ -1,2 +1,2 @@ # Continously build the rojo plugin into the local plugin directory on Windows -rojo build plugin/default.project.json -o $LOCALAPPDATA/Roblox/Plugins/Rojo.rbxm --watch +rojo build plugin/default.project.json -p Rojo.rbxm --watch diff --git a/src/web/api.rs b/src/web/api.rs index 4156af35d..2919e52f3 100644 --- a/src/web/api.rs +++ b/src/web/api.rs @@ -16,7 +16,7 @@ use crate::{ SubscribeMessage, SubscribeResponse, WriteRequest, WriteResponse, PROTOCOL_VERSION, SERVER_VERSION, }, - util::{json, json_ok}, + util::{json, msgpack_ok}, }, }; @@ -58,7 +58,7 @@ impl ApiService { let tree = self.serve_session.tree(); let root_instance_id = tree.get_root_id(); - json_ok(&ServerInfoResponse { + msgpack_ok(&ServerInfoResponse { server_version: SERVER_VERSION.to_owned(), protocol_version: PROTOCOL_VERSION, session_id: self.serve_session.session_id(), @@ -103,7 +103,7 @@ impl ApiService { .map(|patch| SubscribeMessage::from_patch_update(&tree, patch)) .collect(); - json_ok(SubscribeResponse { + msgpack_ok(SubscribeResponse { session_id, message_cursor, messages: api_messages, @@ -159,7 +159,7 @@ impl ApiService { }) .unwrap(); - json_ok(WriteResponse { session_id }) + msgpack_ok(WriteResponse { session_id }) } async fn handle_api_read(&self, request: Request) -> Response { @@ -193,7 +193,7 @@ impl ApiService { } } - json_ok(ReadResponse { + msgpack_ok(ReadResponse { session_id: self.serve_session.session_id(), message_cursor, instances, @@ -271,7 +271,7 @@ impl ApiService { }, }; - json_ok(OpenResponse { + msgpack_ok(OpenResponse { session_id: self.serve_session.session_id(), }) } diff --git a/src/web/util.rs b/src/web/util.rs index 2b6882e5a..7fe4912f6 100644 --- a/src/web/util.rs +++ b/src/web/util.rs @@ -1,8 +1,29 @@ use hyper::{header::CONTENT_TYPE, Body, Response, StatusCode}; use serde::Serialize; -pub fn json_ok(value: T) -> Response { - json(value, StatusCode::OK) +pub fn msgpack_ok(value: T) -> Response { + msgpack(value, StatusCode::OK) +} + +pub fn msgpack(value: T, code: StatusCode) -> Response { + let mut serialized = Vec::new(); + let mut serializer = rmp_serde::Serializer::new(&mut serialized) + .with_human_readable() + .with_struct_map(); + + if let Err(err) = value.serialize(&mut serializer) { + return Response::builder() + .status(StatusCode::INTERNAL_SERVER_ERROR) + .header(CONTENT_TYPE, "text/plain") + .body(Body::from(err.to_string())) + .unwrap(); + }; + + Response::builder() + .status(code) + .header(CONTENT_TYPE, "application/msgpack") + .body(Body::from(serialized)) + .unwrap() } pub fn json(value: T, code: StatusCode) -> Response { From 0d8e1f597bc07b9589b0dcfbadabcaca79083c68 Mon Sep 17 00:00:00 2001 From: nezuo Date: Sat, 4 May 2024 18:45:14 -0700 Subject: [PATCH 04/11] Add nan check to diff --- plugin/src/Reconciler/diff.lua | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/plugin/src/Reconciler/diff.lua b/plugin/src/Reconciler/diff.lua index c87f899c6..0c94e85b2 100644 --- a/plugin/src/Reconciler/diff.lua +++ b/plugin/src/Reconciler/diff.lua @@ -46,6 +46,10 @@ local function trueEquals(a, b): boolean end return true + -- For NaN, check if both values are not equal to themselves + elseif a ~= a and b ~= b then + return true + -- For numbers, compare with epsilon of 0.0001 to avoid floating point inequality elseif typeA == "number" and typeB == "number" then return fuzzyEq(a, b, 0.0001) From 56136be106cc30f5f79c7b6dedfbbf3d7d50d4ef Mon Sep 17 00:00:00 2001 From: nezuo Date: Sat, 4 May 2024 20:16:42 -0700 Subject: [PATCH 05/11] Fix tests --- tests/rojo_test/serve_util.rs | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/tests/rojo_test/serve_util.rs b/tests/rojo_test/serve_util.rs index 6f0490203..58068ca9e 100644 --- a/tests/rojo_test/serve_util.rs +++ b/tests/rojo_test/serve_util.rs @@ -9,6 +9,7 @@ use std::{ use rbx_dom_weak::types::Ref; +use serde::Deserialize; use tempfile::{tempdir, TempDir}; use librojo::web_api::{ReadResponse, ServerInfoResponse, SubscribeResponse}; @@ -145,16 +146,16 @@ impl TestServeSession { pub fn get_api_rojo(&self) -> Result { let url = format!("http://localhost:{}/api/rojo", self.port); - let body = reqwest::blocking::get(&url)?.text()?; + let body = reqwest::blocking::get(url)?.bytes()?; - Ok(serde_json::from_str(&body).expect("Server returned malformed response")) + Ok(deserialize_msgpack(&body).expect("Server returned malformed response")) } pub fn get_api_read(&self, id: Ref) -> Result { let url = format!("http://localhost:{}/api/read/{}", self.port, id); - let body = reqwest::blocking::get(&url)?.text()?; + let body = reqwest::blocking::get(url)?.bytes()?; - Ok(serde_json::from_str(&body).expect("Server returned malformed response")) + Ok(deserialize_msgpack(&body).expect("Server returned malformed response")) } pub fn get_api_subscribe( @@ -162,11 +163,20 @@ impl TestServeSession { cursor: u32, ) -> Result, reqwest::Error> { let url = format!("http://localhost:{}/api/subscribe/{}", self.port, cursor); + let body = reqwest::blocking::get(url)?.bytes()?; - reqwest::blocking::get(&url)?.json() + Ok(deserialize_msgpack(&body).expect("Server returned malformed response")) } } +fn deserialize_msgpack<'a, T: Deserialize<'a>>( + input: &'a [u8], +) -> Result { + let mut deserializer = rmp_serde::Deserializer::new(input).with_human_readable(); + + T::deserialize(&mut deserializer) +} + /// Probably-okay way to generate random enough port numbers for running the /// Rojo live server. /// From 73f6d6ba1b477770f09e5945cba2e6b904241a07 Mon Sep 17 00:00:00 2001 From: nezuo Date: Sun, 5 May 2024 12:19:57 -0700 Subject: [PATCH 06/11] Use msgpack for write request --- plugin/http/init.lua | 5 +++++ plugin/src/ApiContext.lua | 2 +- src/web/api.rs | 4 ++-- src/web/util.rs | 10 +++++++++- tests/rojo_test/serve_util.rs | 2 +- 5 files changed, 18 insertions(+), 5 deletions(-) diff --git a/plugin/http/init.lua b/plugin/http/init.lua index cac0ebc4a..5edca6a72 100644 --- a/plugin/http/init.lua +++ b/plugin/http/init.lua @@ -2,6 +2,7 @@ local HttpService = game:GetService("HttpService") local Promise = require(script.Parent.Promise) local Log = require(script.Parent.Log) +local msgpack = require(script.Parent.msgpack) local HttpError = require(script.Error) local HttpResponse = require(script.Response) @@ -68,4 +69,8 @@ function Http.jsonDecode(source) return HttpService:JSONDecode(source) end +function Http.msgpackEncode(object) + return msgpack.encode(object) +end + return Http diff --git a/plugin/src/ApiContext.lua b/plugin/src/ApiContext.lua index 801137da9..7782dbf9b 100644 --- a/plugin/src/ApiContext.lua +++ b/plugin/src/ApiContext.lua @@ -183,7 +183,7 @@ function ApiContext:write(patch) added = added, } - body = Http.jsonEncode(body) + body = Http.msgpackEncode(body) return Http.post(url, body) :andThen(rejectFailedRequests) diff --git a/src/web/api.rs b/src/web/api.rs index 2919e52f3..baca236fc 100644 --- a/src/web/api.rs +++ b/src/web/api.rs @@ -16,7 +16,7 @@ use crate::{ SubscribeMessage, SubscribeResponse, WriteRequest, WriteResponse, PROTOCOL_VERSION, SERVER_VERSION, }, - util::{json, msgpack_ok}, + util::{deserialize_msgpack, json, msgpack_ok}, }, }; @@ -122,7 +122,7 @@ impl ApiService { let body = body::to_bytes(request.into_body()).await.unwrap(); - let request: WriteRequest = match serde_json::from_slice(&body) { + let request: WriteRequest = match deserialize_msgpack(&body) { Ok(request) => request, Err(err) => { return json( diff --git a/src/web/util.rs b/src/web/util.rs index 7fe4912f6..9f137d52c 100644 --- a/src/web/util.rs +++ b/src/web/util.rs @@ -1,5 +1,5 @@ use hyper::{header::CONTENT_TYPE, Body, Response, StatusCode}; -use serde::Serialize; +use serde::{Deserialize, Serialize}; pub fn msgpack_ok(value: T) -> Response { msgpack(value, StatusCode::OK) @@ -26,6 +26,14 @@ pub fn msgpack(value: T, code: StatusCode) -> Response { .unwrap() } +pub fn deserialize_msgpack<'a, T: Deserialize<'a>>( + input: &'a [u8], +) -> Result { + let mut deserializer = rmp_serde::Deserializer::new(input).with_human_readable(); + + T::deserialize(&mut deserializer) +} + pub fn json(value: T, code: StatusCode) -> Response { let serialized = match serde_json::to_string(&value) { Ok(v) => v, diff --git a/tests/rojo_test/serve_util.rs b/tests/rojo_test/serve_util.rs index 58068ca9e..5298f1e0c 100644 --- a/tests/rojo_test/serve_util.rs +++ b/tests/rojo_test/serve_util.rs @@ -88,7 +88,7 @@ impl TestServeSession { let port_string = port.to_string(); let rojo_process = Command::new(ROJO_PATH) - .args(&[ + .args([ "serve", project_path.to_str().unwrap(), "--port", From 501b67e8b0efeb64d75f31d82449e43a4c5a619d Mon Sep 17 00:00:00 2001 From: nezuo Date: Thu, 9 May 2024 19:44:10 -0700 Subject: [PATCH 07/11] Add different trait to account for nan values --- src/snapshot/patch_compute.rs | 141 +++++++++++++++++++++++++++++++++- 1 file changed, 137 insertions(+), 4 deletions(-) diff --git a/src/snapshot/patch_compute.rs b/src/snapshot/patch_compute.rs index 5d7abc698..5b4034f4e 100644 --- a/src/snapshot/patch_compute.rs +++ b/src/snapshot/patch_compute.rs @@ -6,7 +6,10 @@ use std::{ mem::take, }; -use rbx_dom_weak::types::{Ref, Variant}; +use rbx_dom_weak::types::{ + CFrame, Matrix3, NumberSequenceKeypoint, PhysicalProperties, Ref, UDim, Variant, Vector2, + Vector3, +}; use super::{ patch::{PatchAdd, PatchSet, PatchUpdate}, @@ -122,7 +125,7 @@ fn compute_property_patches( match instance.properties().get(&name) { Some(instance_value) => { - if &snapshot_value != instance_value { + if snapshot_value.different(instance_value) { changed_properties.insert(name, Some(snapshot_value)); } } @@ -224,6 +227,128 @@ fn compute_children_patches( } } +/// Trait where NaN values must not be treated as different. +trait Different { + fn different(&self, b: &Self) -> bool; +} + +impl Different for Variant { + fn different(&self, b: &Self) -> bool { + match (self, b) { + (Variant::CFrame(a), Variant::CFrame(b)) => a.different(b), + (Variant::Float32(a), Variant::Float32(b)) => a.different(b), + (Variant::Float64(a), Variant::Float64(b)) => a.different(b), + (Variant::NumberRange(a), Variant::NumberRange(b)) => { + a.min.different(&b.min) || a.max.different(&b.max) + } + (Variant::NumberSequence(a), Variant::NumberSequence(b)) => { + if a.keypoints.len() != b.keypoints.len() { + return true; + } + + for i in 0..a.keypoints.len() { + if a.keypoints[i].different(&b.keypoints[i]) { + return true; + } + } + + false + } + ( + Variant::PhysicalProperties(PhysicalProperties::Custom(a)), + Variant::PhysicalProperties(PhysicalProperties::Custom(b)), + ) => { + a.density.different(&b.density) + || a.elasticity.different(&b.elasticity) + || a.elasticity_weight.different(&b.elasticity_weight) + || a.friction.different(&b.friction) + || a.friction_weight.different(&b.friction_weight) + } + (Variant::Ray(a), Variant::Ray(b)) => { + a.direction.different(&b.direction) || a.origin.different(&b.origin) + } + (Variant::Rect(a), Variant::Rect(b)) => { + a.min.different(&b.min) || a.max.different(&b.max) + } + (Variant::Region3(a), Variant::Region3(b)) => { + a.min.different(&b.min) || a.max.different(&b.max) + } + (Variant::UDim(a), Variant::UDim(b)) => a.different(b), + (Variant::UDim2(a), Variant::UDim2(b)) => a.x.different(&b.x) || a.y.different(&b.y), + (Variant::Vector2(a), Variant::Vector2(b)) => a.different(b), + (Variant::Vector3(a), Variant::Vector3(b)) => a.different(b), + (Variant::OptionalCFrame(Some(a)), Variant::OptionalCFrame(Some(b))) => a.different(b), + (Variant::Attributes(a), Variant::Attributes(b)) => { + a.len() != b.len() + || a.iter() + .zip(b.iter()) + .any(|((a_name, a_value), (b_name, b_value))| { + a_name != b_name || a_value.different(b_value) + }) + } + _ => self != b, + } + } +} + +impl Different for f32 { + fn different(&self, b: &Self) -> bool { + if self.is_nan() && b.is_nan() { + return false; + } + + self != b + } +} + +impl Different for f64 { + fn different(&self, b: &Self) -> bool { + if self.is_nan() && b.is_nan() { + return false; + } + + self != b + } +} + +impl Different for UDim { + fn different(&self, b: &Self) -> bool { + self.offset != b.offset || self.scale.different(&b.scale) + } +} + +impl Different for Vector2 { + fn different(&self, b: &Self) -> bool { + self.x.different(&b.x) || self.y.different(&b.y) + } +} + +impl Different for Vector3 { + fn different(&self, b: &Self) -> bool { + self.x.different(&b.x) || self.y.different(&b.y) || self.z.different(&b.z) + } +} + +impl Different for CFrame { + fn different(&self, b: &Self) -> bool { + self.position.different(&b.position) || self.orientation.different(&b.orientation) + } +} + +impl Different for Matrix3 { + fn different(&self, b: &Self) -> bool { + self.x.different(&b.x) || self.y.different(&b.y) || self.z.different(&b.z) + } +} + +impl Different for NumberSequenceKeypoint { + fn different(&self, b: &Self) -> bool { + self.envelope.different(&b.envelope) + || self.time.different(&b.time) + || self.value.different(&b.time) + } +} + #[cfg(test)] mod test { use super::*; @@ -246,7 +371,7 @@ mod test { // addition of a prop named Self, which is a self-referential Ref. let snapshot_id = Ref::new(); let snapshot = InstanceSnapshot { - snapshot_id: snapshot_id, + snapshot_id, properties: hashmap! { "Self".to_owned() => Variant::Ref(snapshot_id), }, @@ -288,7 +413,7 @@ mod test { // This patch describes the existing instance with a new child added. let snapshot_id = Ref::new(); let snapshot = InstanceSnapshot { - snapshot_id: snapshot_id, + snapshot_id, children: vec![InstanceSnapshot { properties: hashmap! { "Self".to_owned() => Variant::Ref(snapshot_id), @@ -329,4 +454,12 @@ mod test { assert_eq!(patch_set, expected_patch_set); } + + #[test] + fn different() { + assert!(5.0.different(&6.0)); + assert!(!5.0.different(&5.0)); + assert!(!f32::NAN.different(&f32::NAN)); + assert!(f32::NAN.different(&5.0)); + } } From d7ed3ca9720da988123a3495aca626d104ab130b Mon Sep 17 00:00:00 2001 From: nezuo Date: Mon, 13 May 2024 21:40:01 -0700 Subject: [PATCH 08/11] Use variant_eq instead of Different trait --- Cargo.lock | 10 ++ Cargo.toml | 1 + src/lib.rs | 1 + src/snapshot/patch_compute.rs | 4 +- src/variant_eq.rs | 196 ++++++++++++++++++++++++++++++++++ 5 files changed, 211 insertions(+), 1 deletion(-) create mode 100644 src/variant_eq.rs diff --git a/Cargo.lock b/Cargo.lock index 914ddec5e..631a59a27 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -496,6 +496,15 @@ dependencies = [ "windows-sys 0.52.0", ] +[[package]] +name = "float-cmp" +version = "0.9.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "98de4bbd547a563b716d8dfa9aad1cb19bfab00f4fa09a6a4ed21dbcf44ce9c4" +dependencies = [ + "num-traits", +] + [[package]] name = "fnv" version = "1.0.7" @@ -1763,6 +1772,7 @@ dependencies = [ "csv", "embed-resource", "env_logger", + "float-cmp", "fs-err", "futures", "globset", diff --git a/Cargo.toml b/Cargo.toml index a81422d41..178996a8d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -91,6 +91,7 @@ uuid = { version = "1.7.0", features = ["v4", "serde"] } clap = { version = "3.2.25", features = ["derive"] } profiling = "1.0.15" rmp-serde = "1.3.0" +float-cmp = "0.9.0" [target.'cfg(windows)'.dependencies] winreg = "0.10.1" diff --git a/src/lib.rs b/src/lib.rs index 51195b6c1..0f6fae868 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -20,6 +20,7 @@ mod serve_session; mod session_id; mod snapshot; mod snapshot_middleware; +mod variant_eq; mod web; pub use project::*; diff --git a/src/snapshot/patch_compute.rs b/src/snapshot/patch_compute.rs index 5b4034f4e..2f6906de6 100644 --- a/src/snapshot/patch_compute.rs +++ b/src/snapshot/patch_compute.rs @@ -11,6 +11,8 @@ use rbx_dom_weak::types::{ Vector3, }; +use crate::variant_eq::variant_eq; + use super::{ patch::{PatchAdd, PatchSet, PatchUpdate}, InstanceSnapshot, InstanceWithMeta, RojoTree, @@ -125,7 +127,7 @@ fn compute_property_patches( match instance.properties().get(&name) { Some(instance_value) => { - if snapshot_value.different(instance_value) { + if !variant_eq(&snapshot_value, instance_value) { changed_properties.insert(name, Some(snapshot_value)); } } diff --git a/src/variant_eq.rs b/src/variant_eq.rs new file mode 100644 index 000000000..70f36ea37 --- /dev/null +++ b/src/variant_eq.rs @@ -0,0 +1,196 @@ +use rbx_dom_weak::types::{PhysicalProperties, Variant, Vector3}; + +/// Accepts three argumets: a float type and two values to compare. +/// +/// Returns a bool indicating whether they're equal. This accounts for NaN such +/// that `approx_eq!(f32, f32::NAN, f32::NAN)` is `true`. +macro_rules! approx_eq { + ($Ty:ty, $a:expr, $b:expr) => { + float_cmp::approx_eq!($Ty, $a, $b) || $a.is_nan() && $b.is_nan() + }; +} + +/// Compares two variants to determine if they're equal. This correctly takes +/// float comparisons into account. +pub fn variant_eq(variant_a: &Variant, variant_b: &Variant) -> bool { + if variant_a.ty() != variant_b.ty() { + return false; + } + + match (variant_a, variant_b) { + (Variant::Attributes(a), Variant::Attributes(b)) => { + // If they're not the same size, we can just abort + if a.len() != b.len() { + return false; + } + + // Since Attributes are stored with a BTreeMap, the keys are sorted and we can compare each map's keys in order. + a.iter() + .zip(b.iter()) + .all(|((a_name, a_value), (b_name, b_value))| { + a_name == b_name && variant_eq(a_value, b_value) + }) + } + (Variant::Axes(a), Variant::Axes(b)) => a == b, + (Variant::BinaryString(a), Variant::BinaryString(b)) => a == b, + (Variant::Bool(a), Variant::Bool(b)) => a == b, + (Variant::BrickColor(a), Variant::BrickColor(b)) => a == b, + (Variant::CFrame(a), Variant::CFrame(b)) => { + vector_eq(&a.position, &b.position) + && vector_eq(&a.orientation.x, &b.orientation.x) + && vector_eq(&a.orientation.y, &b.orientation.y) + && vector_eq(&a.orientation.z, &b.orientation.z) + } + (Variant::Color3(a), Variant::Color3(b)) => { + approx_eq!(f32, a.r, b.r) && approx_eq!(f32, a.b, b.b) && approx_eq!(f32, a.g, b.g) + } + (Variant::Color3uint8(a), Variant::Color3uint8(b)) => a == b, + (Variant::ColorSequence(a), Variant::ColorSequence(b)) => { + if a.keypoints.len() != b.keypoints.len() { + return false; + } + let mut a_keypoints = Vec::with_capacity(a.keypoints.len()); + let mut b_keypoints = Vec::with_capacity(b.keypoints.len()); + for keypoint in &a.keypoints { + a_keypoints.push(keypoint) + } + for keypoint in &b.keypoints { + b_keypoints.push(keypoint) + } + a_keypoints.sort_unstable_by(|k1, k2| k1.time.partial_cmp(&k2.time).unwrap()); + b_keypoints.sort_unstable_by(|k1, k2| k1.time.partial_cmp(&k2.time).unwrap()); + + for (a_kp, b_kp) in a_keypoints.iter().zip(b_keypoints) { + if !(approx_eq!(f32, a_kp.time, b_kp.time) + && approx_eq!(f32, a_kp.color.r, b_kp.color.r) + && approx_eq!(f32, a_kp.color.g, b_kp.color.g) + && approx_eq!(f32, a_kp.color.b, b_kp.color.b)) + { + return false; + } + } + true + } + (Variant::Content(a), Variant::Content(b)) => a == b, + (Variant::Enum(a), Variant::Enum(b)) => a == b, + (Variant::Faces(a), Variant::Faces(b)) => a == b, + (Variant::Float32(a), Variant::Float32(b)) => approx_eq!(f32, *a, *b), + (Variant::Float64(a), Variant::Float64(b)) => approx_eq!(f64, *a, *b), + (Variant::Font(a), Variant::Font(b)) => { + a.weight == b.weight + && a.style == b.style + && a.family == b.family + && a.cached_face_id == b.cached_face_id + } + (Variant::Int32(a), Variant::Int32(b)) => a == b, + (Variant::Int64(a), Variant::Int64(b)) => a == b, + (Variant::MaterialColors(a), Variant::MaterialColors(b)) => a.encode() == b.encode(), + (Variant::NumberRange(a), Variant::NumberRange(b)) => { + approx_eq!(f32, a.max, b.max) && approx_eq!(f32, a.min, a.max) + } + (Variant::NumberSequence(a), Variant::NumberSequence(b)) => { + if a.keypoints.len() != b.keypoints.len() { + return false; + } + let mut a_keypoints = Vec::with_capacity(a.keypoints.len()); + let mut b_keypoints = Vec::with_capacity(b.keypoints.len()); + for keypoint in &a.keypoints { + a_keypoints.push(keypoint) + } + for keypoint in &b.keypoints { + b_keypoints.push(keypoint) + } + a_keypoints.sort_unstable_by(|k1, k2| k1.time.partial_cmp(&k2.time).unwrap()); + b_keypoints.sort_unstable_by(|k1, k2| k1.time.partial_cmp(&k2.time).unwrap()); + for (a_kp, b_kp) in a_keypoints.iter().zip(b_keypoints) { + if !(approx_eq!(f32, a_kp.time, b_kp.time) + && approx_eq!(f32, a_kp.value, b_kp.value) + && approx_eq!(f32, a_kp.envelope, b_kp.envelope)) + { + return false; + } + } + true + } + (Variant::OptionalCFrame(a), Variant::OptionalCFrame(b)) => { + if let (Some(a2), Some(b2)) = (a, b) { + vector_eq(&a2.position, &b2.position) + && vector_eq(&a2.orientation.x, &b2.orientation.x) + && vector_eq(&a2.orientation.y, &b2.orientation.y) + && vector_eq(&a2.orientation.z, &b2.orientation.z) + } else { + false + } + } + (Variant::PhysicalProperties(a), Variant::PhysicalProperties(b)) => match (a, b) { + (PhysicalProperties::Default, PhysicalProperties::Default) => true, + (PhysicalProperties::Custom(a2), PhysicalProperties::Custom(b2)) => { + approx_eq!(f32, a2.density, b2.density) + && approx_eq!(f32, a2.elasticity, b2.elasticity) + && approx_eq!(f32, a2.friction, b2.friction) + && approx_eq!(f32, a2.elasticity_weight, b2.elasticity_weight) + && approx_eq!(f32, a2.friction_weight, b2.friction_weight) + } + (_, _) => false, + }, + (Variant::Ray(a), Variant::Ray(b)) => { + vector_eq(&a.direction, &b.direction) && vector_eq(&a.origin, &b.origin) + } + (Variant::Rect(a), Variant::Rect(b)) => { + approx_eq!(f32, a.max.x, b.max.x) + && approx_eq!(f32, a.max.y, b.max.y) + && approx_eq!(f32, a.min.x, b.min.x) + && approx_eq!(f32, a.min.y, b.min.y) + } + (Variant::Ref(a), Variant::Ref(b)) => a == b, + (Variant::Region3(a), Variant::Region3(b)) => { + vector_eq(&a.max, &b.max) && vector_eq(&a.min, &b.min) + } + (Variant::Region3int16(a), Variant::Region3int16(b)) => a == b, + (Variant::SecurityCapabilities(a), Variant::SecurityCapabilities(b)) => a == b, + (Variant::SharedString(a), Variant::SharedString(b)) => a == b, + (Variant::Tags(a), Variant::Tags(b)) => { + let mut a_sorted: Vec<&str> = a.iter().collect(); + let mut b_sorted: Vec<&str> = b.iter().collect(); + if a_sorted.len() == b_sorted.len() { + a_sorted.sort_unstable(); + b_sorted.sort_unstable(); + for (a_tag, b_tag) in a_sorted.into_iter().zip(b_sorted) { + if a_tag != b_tag { + return false; + } + } + true + } else { + false + } + } + (Variant::UDim(a), Variant::UDim(b)) => { + approx_eq!(f32, a.scale, b.scale) && a.offset == b.offset + } + (Variant::UDim2(a), Variant::UDim2(b)) => { + approx_eq!(f32, a.x.scale, b.x.scale) + && a.x.offset == b.x.offset + && approx_eq!(f32, a.y.scale, b.y.scale) + && a.y.offset == b.y.offset + } + (Variant::UniqueId(a), Variant::UniqueId(b)) => a == b, + (Variant::String(a), Variant::String(b)) => a == b, + (Variant::Vector2(a), Variant::Vector2(b)) => { + approx_eq!(f32, a.x, b.x) && approx_eq!(f32, a.y, b.y) + } + (Variant::Vector2int16(a), Variant::Vector2int16(b)) => a == b, + (Variant::Vector3(a), Variant::Vector3(b)) => vector_eq(a, b), + (Variant::Vector3int16(a), Variant::Vector3int16(b)) => a == b, + (a, b) => panic!( + "unsupport variant comparison: {:?} and {:?}", + a.ty(), + b.ty() + ), + } +} + +#[inline(always)] +fn vector_eq(a: &Vector3, b: &Vector3) -> bool { + approx_eq!(f32, a.x, b.x) && approx_eq!(f32, a.y, b.y) && approx_eq!(f32, a.z, b.z) +} From 09a97eb982c2ab6994ea2e0d737d9f5d3d393351 Mon Sep 17 00:00:00 2001 From: nezuo Date: Mon, 13 May 2024 21:42:02 -0700 Subject: [PATCH 09/11] Update changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index aec5f7ab2..90300a04f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -54,6 +54,7 @@ | `ignore` | None! | **All** sync rules are reset between project files, so they must be specified in each one when nesting them. This is to ensure that nothing can break other projects by changing how files are synced! +* `inf` and `nan` values in properties are now synced. ([#908]) [#813]: https://github.com/rojo-rbx/rojo/pull/813 [#834]: https://github.com/rojo-rbx/rojo/pull/834 @@ -62,6 +63,7 @@ [#883]: https://github.com/rojo-rbx/rojo/pull/883 [#893]: https://github.com/rojo-rbx/rojo/pull/893 [#903]: https://github.com/rojo-rbx/rojo/pull/903 +[#908]: https://github.com/rojo-rbx/rojo/pull/908 [#911]: https://github.com/rojo-rbx/rojo/pull/911 ## [7.4.1] - February 20, 2024 From d4df9bdf5dd1daee11d0d5953e5e2330140c681d Mon Sep 17 00:00:00 2001 From: nezuo Date: Mon, 13 May 2024 21:42:40 -0700 Subject: [PATCH 10/11] Remove period --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 90300a04f..8cb11ac05 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -54,7 +54,7 @@ | `ignore` | None! | **All** sync rules are reset between project files, so they must be specified in each one when nesting them. This is to ensure that nothing can break other projects by changing how files are synced! -* `inf` and `nan` values in properties are now synced. ([#908]) +* `inf` and `nan` values in properties are now synced ([#908]) [#813]: https://github.com/rojo-rbx/rojo/pull/813 [#834]: https://github.com/rojo-rbx/rojo/pull/834 From eaf86723428bba71cab5ce54e852b85abf21fdbe Mon Sep 17 00:00:00 2001 From: nezuo Date: Wed, 15 May 2024 10:10:34 -0700 Subject: [PATCH 11/11] Remove Different trait --- src/snapshot/patch_compute.rs | 135 +--------------------------------- 1 file changed, 1 insertion(+), 134 deletions(-) diff --git a/src/snapshot/patch_compute.rs b/src/snapshot/patch_compute.rs index 2f6906de6..f59f828be 100644 --- a/src/snapshot/patch_compute.rs +++ b/src/snapshot/patch_compute.rs @@ -6,10 +6,7 @@ use std::{ mem::take, }; -use rbx_dom_weak::types::{ - CFrame, Matrix3, NumberSequenceKeypoint, PhysicalProperties, Ref, UDim, Variant, Vector2, - Vector3, -}; +use rbx_dom_weak::types::{Ref, Variant}; use crate::variant_eq::variant_eq; @@ -229,128 +226,6 @@ fn compute_children_patches( } } -/// Trait where NaN values must not be treated as different. -trait Different { - fn different(&self, b: &Self) -> bool; -} - -impl Different for Variant { - fn different(&self, b: &Self) -> bool { - match (self, b) { - (Variant::CFrame(a), Variant::CFrame(b)) => a.different(b), - (Variant::Float32(a), Variant::Float32(b)) => a.different(b), - (Variant::Float64(a), Variant::Float64(b)) => a.different(b), - (Variant::NumberRange(a), Variant::NumberRange(b)) => { - a.min.different(&b.min) || a.max.different(&b.max) - } - (Variant::NumberSequence(a), Variant::NumberSequence(b)) => { - if a.keypoints.len() != b.keypoints.len() { - return true; - } - - for i in 0..a.keypoints.len() { - if a.keypoints[i].different(&b.keypoints[i]) { - return true; - } - } - - false - } - ( - Variant::PhysicalProperties(PhysicalProperties::Custom(a)), - Variant::PhysicalProperties(PhysicalProperties::Custom(b)), - ) => { - a.density.different(&b.density) - || a.elasticity.different(&b.elasticity) - || a.elasticity_weight.different(&b.elasticity_weight) - || a.friction.different(&b.friction) - || a.friction_weight.different(&b.friction_weight) - } - (Variant::Ray(a), Variant::Ray(b)) => { - a.direction.different(&b.direction) || a.origin.different(&b.origin) - } - (Variant::Rect(a), Variant::Rect(b)) => { - a.min.different(&b.min) || a.max.different(&b.max) - } - (Variant::Region3(a), Variant::Region3(b)) => { - a.min.different(&b.min) || a.max.different(&b.max) - } - (Variant::UDim(a), Variant::UDim(b)) => a.different(b), - (Variant::UDim2(a), Variant::UDim2(b)) => a.x.different(&b.x) || a.y.different(&b.y), - (Variant::Vector2(a), Variant::Vector2(b)) => a.different(b), - (Variant::Vector3(a), Variant::Vector3(b)) => a.different(b), - (Variant::OptionalCFrame(Some(a)), Variant::OptionalCFrame(Some(b))) => a.different(b), - (Variant::Attributes(a), Variant::Attributes(b)) => { - a.len() != b.len() - || a.iter() - .zip(b.iter()) - .any(|((a_name, a_value), (b_name, b_value))| { - a_name != b_name || a_value.different(b_value) - }) - } - _ => self != b, - } - } -} - -impl Different for f32 { - fn different(&self, b: &Self) -> bool { - if self.is_nan() && b.is_nan() { - return false; - } - - self != b - } -} - -impl Different for f64 { - fn different(&self, b: &Self) -> bool { - if self.is_nan() && b.is_nan() { - return false; - } - - self != b - } -} - -impl Different for UDim { - fn different(&self, b: &Self) -> bool { - self.offset != b.offset || self.scale.different(&b.scale) - } -} - -impl Different for Vector2 { - fn different(&self, b: &Self) -> bool { - self.x.different(&b.x) || self.y.different(&b.y) - } -} - -impl Different for Vector3 { - fn different(&self, b: &Self) -> bool { - self.x.different(&b.x) || self.y.different(&b.y) || self.z.different(&b.z) - } -} - -impl Different for CFrame { - fn different(&self, b: &Self) -> bool { - self.position.different(&b.position) || self.orientation.different(&b.orientation) - } -} - -impl Different for Matrix3 { - fn different(&self, b: &Self) -> bool { - self.x.different(&b.x) || self.y.different(&b.y) || self.z.different(&b.z) - } -} - -impl Different for NumberSequenceKeypoint { - fn different(&self, b: &Self) -> bool { - self.envelope.different(&b.envelope) - || self.time.different(&b.time) - || self.value.different(&b.time) - } -} - #[cfg(test)] mod test { use super::*; @@ -456,12 +331,4 @@ mod test { assert_eq!(patch_set, expected_patch_set); } - - #[test] - fn different() { - assert!(5.0.different(&6.0)); - assert!(!5.0.different(&5.0)); - assert!(!f32::NAN.different(&f32::NAN)); - assert!(f32::NAN.different(&5.0)); - } }