feat: Update reconciler / reify to use CreateMeshPartAsync to create mesh parts #534

unix-system · 2022-04-18T22:29:28Z

Closes #402.

Problem

Currently, Rojo does not support creation of some instances and properties via the Studio plugin - notably UnionOperations and MeshParts. MeshParts can in fact be created by Plugins through the InsertService.CreateMeshPartAsync method at Plugin security.

This impacts developers using Rojo to work, as it prevents them from live importing any such instances (and can only be created from a file via rojo build or manual import).

Solution

This pull request exclusively modifies the Lua plugin; no changes were necessary to the Rojo binary itself. The changes have a soft dependency on a pull request to rbx-dom to update the database files, although this is not a requirement.

The primary change was to modify the reify function to include a switch for custom instance creation logic which could be expanded in the future if necessary.

This change was coupled closely with the implementation of the requiresRecreate property of ApiInstanceUpdate objects, which is a partial replacement for the changedClassName property. This property triggers a condition in the applyPatch function to recreate the instance using reify if the requiresRecreate property is present (previously done for the changedClassName property).

The requiresRecreate property is set during the creation of an update patch (either in batch, in encodePatchUpdate, or during diff).

Tests

Tests have been implemented which support the new feature being added. A dedicated test project (test-projects/mesh_parts/default.project.json) has been included with two MeshParts (one rbxm and one rbxmx), of the Utah Teapot.

Other

The development version of Rojo was bumped to 7.0.0 (latest).

unix-system · 2022-04-18T23:19:56Z

plugin/src/ChangeBatcher/encodePatchUpdate.lua

 	}

 	for propertyName in pairs(properties) do
 		if propertyName == "Name" then
 			update.changedName = instance.Name
-		else


This change, whilst structural (it now encodes the Name field as well as setting the changedName property) has not changed the intended logic and is more concise.

LPGhatguy

PR looks good, thanks for the submission!

Can you back out the change to database.json? We should be able to land any updates to rbx_dom_lua in a separate PR and it is a frequent source of merge conflicts.

I mentioned in the review comments, but I think we should remove requiresRecreate from the patch type and instead calculate it during applyPatch. The Rojo server produces most of the patches that the plugin receives and doesn't have any knowledge of what would require an instance to be recreated. It's bets if we let the plugin decide for itself when it goes to apply a change.

LPGhatguy · 2022-06-12T23:38:04Z

plugin/src/Reconciler/reify.lua

+	-- Custom logic for MeshParts
+	-- see: https://github.com/rojo-rbx/rojo/issues/402
+	if className == "MeshPart" then
+		local okMeshId, meshId = decodeValue(properties.MeshId, instanceMap)


We need to handle okMeshId, because the second return value will not be a mesh ID if this failed to decode.

LPGhatguy · 2022-06-12T23:39:37Z

plugin/src/Types.lua

@@ -25,6 +25,7 @@ local ApiInstanceUpdate = t.interface({
 	id = RbxId,
 	changedName = t.optional(t.string),
 	changedClassName = t.optional(t.string),
+	requiresRecreate = t.optional(t.boolean),


I don't think we should update this interface at all. The server generates most of the patches that the Rojo plugin handles and it does not populate requiresRecreate. We can calculate this when we go to apply a patch instead.

LPGhatguy · 2022-06-12T23:41:47Z

plugin/src/Reconciler/applyPatch.lua

+		-- If the instance's className changed, or there is custom recreate logic,
+		-- we have a bumpy ride ahead while we recreate this instance and move all 
+		-- of its children into the new version atomically...ish.
+		if update.requiresRecreate then


There are comments in this block of code about its caveats, notably not preserving existing properties if we end up needing to recreate an instance. It's probably not the end of the world, but some of the comments I left about this mostly occurring during Folder<->ModuleScript transitions are probably not accurate anymore.

LPGhatguy · 2022-06-12T23:42:26Z

plugin/src/ChangeBatcher/encodePatchUpdate.spec.lua

+	it("should recreate instance in the update when the instance's ClassName changes", function()
+		local part = Instance.new("Part")
+		local properties = {
+			ClassName = true


We should set this value to a real ClassName so that future validation in the patch update function won't choke when it sees a bool instead.

LPGhatguy · 2022-06-12T23:43:10Z

plugin/src/Reconciler/applyPatch.spec.lua

@@ -156,7 +158,33 @@ return function()
 		expect(value.Value).to.equal("WORLD")
 	end)

-	it("should recreate instances when changedClassName is set, preserving children", function()
+	it("should recreate instances for MeshId updates", function()


This is a great test!

LPGhatguy · 2022-06-12T23:45:22Z

plugin/src/Reconciler/applyPatch.lua

+		-- If the instance's className changed, or there is custom recreate logic,
+		-- we have a bumpy ride ahead while we recreate this instance and move all 
+		-- of its children into the new version atomically...ish.
+		if update.requiresRecreate then


Instead of having requiresRecreate as a property here, we should check the conditions that would cause us to recreate an instance directly. For now, that's just changedClassName ~= nil or changedProperties["MeshId"] ~= nil.

LPGhatguy · 2022-06-12T23:47:04Z

plugin/src/Reconciler/reify.lua

+
+		-- It is okay to provide default properties here, because 
+		-- they will be overriden on the update cycle
+		return InsertService:CreateMeshPartAsync(meshId, Enum.CollisionFidelity.Default, Enum.RenderFidelity.Automatic)


This is the first time we'll be introducing an async function into Rojo's reconciler bits. This is a possible source of bugs that we should watch out for in the future.

LPGhatguy · 2022-06-12T23:47:28Z

plugin/src/Reconciler/reify.lua

+		-- It is okay to provide default properties here, because 
+		-- they will be overriden on the update cycle


Suggested change

-- It is okay to provide default properties here, because

-- they will be overriden on the update cycle

-- It is okay to provide default properties here, because

-- they will be set when properties are applied in reify.

GreenAppers · 2023-12-02T23:51:31Z

Hm. Why was this closed?

sasial-dev · 2023-12-02T23:52:24Z

It was closed in favour of #786.

unix-system marked this pull request as ready for review April 18, 2022 23:18

unix-system commented Apr 18, 2022

View reviewed changes

LPGhatguy requested changes Jun 12, 2022

View reviewed changes

LPGhatguy added the status: needs review This work is mostly done, but just needs work to integrate and merge it. label Jun 29, 2022

unix-system added 8 commits November 15, 2023 10:35

Add test teapot

2db3c20

Add test project for MeshParts

bc770de

Add new database

2d79607

Introduce new "requiresRecreate" property in place of changedClassName

abe69dd

Change logic to recreate when MeshId is changed

4e19227

Update tests

29594f1

Main work: Use CreateMeshPartAsync to create mesh parts

4f4b8ec

Introduce appropriate new tests for requiresRecreate

6c1beb9

unix-system force-pushed the feat/createmeshpartasync branch from 50d350c to 6c1beb9 Compare November 15, 2023 10:39

Pull database.json from rbx-dom-lua

defed06

unix-system marked this pull request as draft November 15, 2023 10:47

unix-system closed this Nov 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Update reconciler / reify to use CreateMeshPartAsync to create mesh parts #534

feat: Update reconciler / reify to use CreateMeshPartAsync to create mesh parts #534

unix-system commented Apr 18, 2022 •

edited

Loading

unix-system Apr 18, 2022

LPGhatguy left a comment

LPGhatguy Jun 12, 2022

LPGhatguy Jun 12, 2022

LPGhatguy Jun 12, 2022

LPGhatguy Jun 12, 2022

LPGhatguy Jun 12, 2022

LPGhatguy Jun 12, 2022

LPGhatguy Jun 12, 2022

LPGhatguy Jun 12, 2022

GreenAppers commented Dec 2, 2023

sasial-dev commented Dec 2, 2023

		-- It is okay to provide default properties here, because
		-- they will be overriden on the update cycle

feat: Update reconciler / reify to use CreateMeshPartAsync to create mesh parts #534

feat: Update reconciler / reify to use CreateMeshPartAsync to create mesh parts #534

Conversation

unix-system commented Apr 18, 2022 • edited Loading

Problem

Solution

Tests

Other

Choose a reason for hiding this comment

LPGhatguy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GreenAppers commented Dec 2, 2023

sasial-dev commented Dec 2, 2023

unix-system commented Apr 18, 2022 •

edited

Loading