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

Add Property Migration #253

Closed
wants to merge 28 commits into from

Conversation

Corecii
Copy link
Member

@Corecii Corecii commented Mar 12, 2023

Note: this depends on the Implement Font Type branch #248 because Font is one of the major reasons that we need migrations.

If you'd like, I can rip out the Font-specific parts of this code so it can be easily compared to the master branch, but I'd prefer if Font support were merged first so that wasn't a concern. Font support looks ready, though it needs one fix for an edge case. We could merge the Font PR now, then write the edge-case fix as a separate PR.

A diff between this branch and the Font branch can be found here: UpliftGames#1

Preview

This PR allows us to specify migrations using the patches files and a Variant to Variant converter in the actual Rust code. This is better than having the Rust code do hard-coded checks against property names and such without being super complex to implement or think about. Writing new migrations this way is easy!

The patch files look like this:

Change:
  TextLabel:
    Font:
      Serialization:
        Type: Migrate
        Property: FontFace
        Migration: FontToFontFace

I've chosen to have migrations not be performed if the target property already exists. This matches Roblox's behavior where new properties always take priority. I figure making priority work both ways would be complex, and since backwards priority isn't known behavior of any present-day properties, it's unnecessary complexity. There's room to expand this in the future if we need to.

Why do we need this?

Two new properties -- FontFace and ScreenInsets -- are migrated from old properties. When both the old and new properties are present, Roblox always prefers the new properties.

The Roblox binary format requires that all objects of the same class have the same properties present. When mixing old models and new models into the same file using Rojo or Remodel, old models get assigned defaults for the new properties, which then take priority over the old properties.

This has two clear affects that users are actively experiencing in "preview" builds of Rojo and Remodel:

  • Mixing old and new models with UI via rojo build or Remodel scripts results in all old text UI having the Arial font.
  • Mixing old and new models with ScreenGuis via rojo build or Remodel scripts results in all old ScreenGuis losing their IgnoreGuiInset value.

The only work-around for users right now is to open their models in Roblox and re-save over them to get Roblox to do the migration. This is not a reasonable ask of a Rojo fully-managed project that may have hundreds of models.

As time goes on, more properties will get migrations like this and maintaining a fully-managed Rojo codebase will become an unreasonable task.

Implementing these migrations on the rbx-dom side is quite easy. If we don't do it, either someone else will add this feature themselves or people just won't do fully-managed Rojo due to the high risk of breakage.

Changes in this PR

  • Adds a new type of PropertySerialization, Migrate, where you specify the new property name to migrate to and the name of the migration.
  • Adds named migrations, which convert from one Variant to another Variant.
  • Changes deserializer code to respect the Migrate type.
  • Adds tests for these new migrations.
  • Adds documentation instructing how to add a new migration.

@Dekkonot
Copy link
Member

Dekkonot commented May 1, 2023

This PR is in a weird spot right now because (I think) of how we squash PRs when we merge them. It's listing a lot of changes to files that already performed in the Font implementation PR, and as a result we have some very... fun to fix merge conflicts that I'm not sure you'll be able to cleanly fix without force pushing to this branch. Interested to know if you think you can manage it or if you just wanna fast forward this branch and re-add the relevant code.

With regards to the implementation, this fits neatly into the existing patch system but I worry that we might be outgrowing that system altogether. yaml isn't the most understandable format to begin with and as we begin adding more patches (and probably move patches around because this system lets us make it more sane), it might get worryingly hard to maintain and read. I don't want to suggest we move to a new system because that's quite frankly out of scope for this PR, but I'd love to hear your opinion either here or on Discord about what the future of the patches for the database look like.

@Corecii
Copy link
Member Author

Corecii commented May 2, 2023

I'll clean this up this week. I have a lot going on right now but I'm starting to get a bit of extra time available again.


I think the maintenance burden you're worried about running into is largely because "yaml isn't the most understandable format to begin with". I wouldn't be opposed to moving to an easier-to-understand format. A large part of that, to me, is good documentation and clear structure. The amount of nesting that patch tanks have is part of what makes them difficult to understand to me.

I'm not sure what else would be good to move to. Json is too strict. Would toml be too simple?

[Change.TextLabel.Font.Serialization]
Type = "Migrate"
Property = "FontFace"
Migration = "FontToFontFace"

I think that would be approximately equivalent to the yaml in this PR. To me that looks a lot easier to maintain. It's also easier for us to move it around since it's "flat" and "portable" between patch files.

To think of something entirely new, I'll need to sit down and take a look at what the system is like right now and what our upcoming needs are. I feel like maybe patches are reasonable for what they do now, and migrations push their bounds because they would be better as their own thing, like Lucien suggested.

I think making sure patches are organized in a good manner is a big part of keeping it maintainable too, so being able to easily move patches around would be beneficial.

We should probably make an issue for this to track our thoughts and potential solutions.

@Corecii
Copy link
Member Author

Corecii commented May 12, 2023

This is a pain to get to a point where it reflects the most recent changes properly and diffs properly. I'm just going to close this PR and open a new one that isn't cursed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants