feat: add --base flag to upload command for merging with place files#1226
feat: add --base flag to upload command for merging with place files#1226ardrous wants to merge 3 commits intorojo-rbx:masterfrom
Conversation
Dekkonot
left a comment
Dekkonot
left a comment
There was a problem hiding this comment.
Thanks for the PR! A few things:
- Supporting this just for
rojo uploadisn't desirable. Adding support torojo buildwould make this significantly easier to test, and improve the usability. - This is akin to just putting a Rojo project over top a Roblox file. This means that unknown instances aren't deleted. This could be a problem if you were e.g. updating packages and expecting this command to take care any lingering files.
- There aren't any tests for this to verify it's working. Writing tests for
rojo uploadis on the agenda still, but it'd be nice to at least verify the outputted rbxl is what you want it to be (see: adding support for this torojo build) - I have performance concerns with the recursive design here. Rojo projects and Roblox games can hundreds of thousands of Instances in them and Instance trees can be arbitrarily deep. Those combine to make recursive navigation of the DataModel a problem.
- We support uploading model files, so limiting
--baseto only using rbxl and rbxlx files is wrong.
|
Thank you for your swift response, I have made the changes based on your feedback. I ran the tests and they all work fine, and I have been using it on one of my games and it has been working perfectly without any issues. If there are any issues let me know. |
Dekkonot
left a comment
Dekkonot
left a comment
There was a problem hiding this comment.
Overall looks good, but I think we should try to make the behavior for models and places behave the same.
| } | ||
|
|
||
| /// Reads a base Roblox file (.rbxl, .rbxlx, .rbxm, .rbxmx) into a WeakDom. | ||
| pub fn read_base_dom(path: &Path) -> anyhow::Result<WeakDom> { |
There was a problem hiding this comment.
This code is essentially identical to code that exists in the syncback CLI but it doesn't do any of the required post-processing for model files that is generally required. While I have long-term ambitions to move the code elsewhere, for now this should probably just be a 1:1 rip of the code from syncback. It simplifies the logic by making it so you don't have to treat models and places differently when performing matching.
| /// For place projects (root class is "DataModel"), iteratively merges Rojo's | ||
| /// children into matching base children by name and className. For model | ||
| /// projects, clones the entire Rojo tree as a child of the base root. |
There was a problem hiding this comment.
Once the read_base_dom function is changed to pre-process model DOMS, we can just treat them the same. This is almost certainly the desired and expected behavior for merging a Rojo project into a file anyway, so it works out.
| let ignore_unknown = rojo | ||
| .get_metadata(pair.rojo_ref) | ||
| .map(|m| m.ignore_unknown_instances) | ||
| .unwrap_or(true); |
There was a problem hiding this comment.
The default for ignore_unknown_instances is false, not true.
| // For model projects (root is not DataModel), clone entire Rojo tree | ||
| // as a child of the base root. | ||
| if rojo_root.class.as_str() != "DataModel" { | ||
| let cloned_ref = rojo_inner.clone_into_external(rojo_root_id, &mut base); | ||
| base.transfer_within(cloned_ref, base_root_ref); | ||
| return Ok(base); | ||
| } |
There was a problem hiding this comment.
This behavior is entirely unexpected and should be removed. I would rather this behave the same for both models and places.
| }]; | ||
|
|
||
| while let Some(pair) = stack.pop() { | ||
| let rojo_children: Vec<Ref> = rojo_inner |
There was a problem hiding this comment.
This doesn't need to be a vector.
2 participants
Currently,
rojo uploadbuilds the project and does not take any pre-existing place files. My implementation is to add a --base flag for backwards compatibility.