When a patch fails, import the Instance directly using GetObjects
#786
Conversation
this was widely regarded as a bad move
As opposed to a temp dir and hardlinks
adds stylua pr to ignore so it doesn't show on git blame history
|
That's my bad @Barocena, I forgot to specify a flag when merging master into my branch! |
This comment was marked as spam.
This comment was marked as spam.
| -- `ReferentMap` is a folder that contains nothing but | ||
| -- ObjectValues which will be named after entries in `instanceMap` | ||
| -- and have their `Value` property point towards a new Instance | ||
| -- that it can be swapped out with. In turn, `reified` is a | ||
| -- container for all of the objects pointed to by those instances. |
There was a problem hiding this comment.
Not sure ReferentMap is necessary - it seems a little complex for what it's actually accomplishing
Because our serializers provide ordering guarantees for children, and GetObject insertion reflects this ordering, I think the server could construct a model file with children in the same order as the requested IDs, and then the client can match them back up via this ordering.
Alternatively, if it's uncomfortable to rely on child order this way, then the server could write multiple files, and respond with multiple paths (again, in the same order as the requested IDs), at the expense of multiple file system writes and multiple calls to GetObjects
There was a problem hiding this comment.
I went with ReferentMap because it was the first solution I thought of that avoided a bunch of FS writes and reads, not necessarily because I thought it was the best option. In practice it's not really all that complicated, it just sounds bad when you write it out. I can get behind wanting to simplify it as much as possible though, since it's unusual for Rojo.
There's the potential to write thousands of files to a file system and then load them in very quickly if we use multiple paths. I'm not inherently opposed to that, but I'd want to check what the performance of that was. It's not great as-is with one monolith model, I can imagine it getting really bad with a bunch of them.
I'm going to veto relying on child ordering just on principle. It makes me feel gross.
There was a problem hiding this comment.
I think I overlooked the additional complication caused by instances with parent restrictions, so ordering wouldn't work anyway... it's probably fine
| if PatchSet.hasUpdates(unappliedPatch) then | ||
| local idList = table.create(#unappliedPatch.updated) | ||
| for i, entry in unappliedPatch.updated do | ||
| idList[i] = entry.id |
There was a problem hiding this comment.
Should also check here that the unapplied updates are for properties, otherwise we may try to fetch instances for updates (like metadata) that GetObjects won't help with
| -- TODO this is destructive to any properties that are | ||
| -- overwritten by the user but not known to Rojo. We can probably | ||
| -- mitigate that by keeping tabs of any instances we need to swap. | ||
| fetchInstances(idList, self.__instanceMap, self.__apiContext) |
There was a problem hiding this comment.
I don't know if it makes sense to be performing HTTP requests in the reconciler. I suppose the entire fetch process could conceptually be seen as part of patch application, but I think I'd rather have the API call happen in ServeSession (maybe by wrapping Reconciler:applyPatch in a new function ServeSession:__applyPatch) to better follow the existing architecture, to make the control flow a little clearer, and to avoid passing an ApiContext plus a setting to the reconciler
There was a problem hiding this comment.
I don't love it, but I'll just be honest: this was exploratory surgery on the plugin. I didn't really know where to put it, so I went with the reconciler because it was at least sensible.
Closes #205.
This introduces new API that allows for the plugin to request a list of Instances via their ID to be built and placed into the content folder and then changes the plugin to use this API whenever a patch fails.
There are currently flaws in this design, which is why this PR is marked as a draft. They are, namely:
fetchendpoint are not cleaned up yetThese are all things I intend to resolve, but they shouldn't substantially alter the base implementation.
Of interest is the design of the
fetchendpoint. Due to the fact that Instance references cannot be communicated between the client and the server, some method had to be invented to link any fetched instance to its internal ID.The obvious way to handle this is to just name every Instance after its ID and then have the plugin rename the Instance during the replacement process. This would work well, except that some Instances have specific parent requirements. If as an example, a
Boneis requested from this endpoint, it has to be parented to aMeshPartin order for Studio to load the model. So, the suddenly simple design becomes more complicated because it's no longer a 1:1 mapping. A work around could be made, but I opted to instead just design around the problem.The actual design uses
ObjectValuesto point to the actual object we're using to swap a part out rather than name. The resulting structure looks like this:Every
ObjectValueunderReferentMapis named after an internal Instance ID and has a value that points to its replacement, which is parented underReified. This is smooth to navigate in code and easy to debug in the worst case.