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

Use msgpack for API #908

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitmodules
Original file line number Diff line number Diff line change
Expand Up @@ -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
9 changes: 5 additions & 4 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
6 changes: 3 additions & 3 deletions aftman.toml
Original file line number Diff line number Diff line change
@@ -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"
5 changes: 5 additions & 0 deletions build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,11 @@ fn snapshot_from_fs_path(path: &Path) -> io::Result<VfsSnapshot> {
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));
}
Expand Down
1 change: 1 addition & 0 deletions plugin/Packages/msgpack-luau
Submodule msgpack-luau added at 0754b1
8 changes: 7 additions & 1 deletion plugin/http/Response.lua
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
local HttpService = game:GetService("HttpService")

local msgpack = require(script.Parent.Parent.msgpack)

local stringTemplate = [[
Http.Response {
code: %d
Expand Down Expand Up @@ -31,4 +33,8 @@ function Response:json()
return HttpService:JSONDecode(self.body)
end

return Response
function Response:msgpack()
return msgpack.decode(self.body)
end

return Response
5 changes: 5 additions & 0 deletions plugin/http/init.lua
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -68,4 +69,8 @@ function Http.jsonDecode(source)
return HttpService:JSONDecode(source)
end

function Http.msgpackEncode(object)
return msgpack.encode(object)
end

return Http
21 changes: 12 additions & 9 deletions plugin/src/ApiContext.lua
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand All @@ -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
Expand Down Expand Up @@ -183,13 +183,16 @@ function ApiContext:write(patch)
added = added,
}

body = Http.jsonEncode(body)
body = Http.msgpackEncode(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()
Expand All @@ -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
Expand All @@ -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
Expand Down
4 changes: 4 additions & 0 deletions plugin/src/Reconciler/diff.lua
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion plugin/watch-build.sh
Original file line number Diff line number Diff line change
@@ -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
141 changes: 137 additions & 4 deletions src/snapshot/patch_compute.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand Down Expand Up @@ -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));
}
}
Expand Down Expand Up @@ -224,6 +227,128 @@ fn compute_children_patches(
}
}

/// Trait where NaN values must not be treated as different.
trait Different {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole trait is... not ideal. I would strongly prefer we move the equality check for Variant to its own file, and make it a simple function instead of a trait.

I have a function already implemented in my syncback fork here if you want to steal it.

It does require us to pull in float_cmp as a crate, but I think that's fine.

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::*;
Expand All @@ -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),
},
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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));
}
}
Loading
Loading