wiwei commented 5 years ago

Filing an issue to that we need to have a proposal written for how we're going to manage breaking changes and other migitations that we need to provide to customers as they upgrade from build to build.

The nature of the MRTK is one where MUCH of the code is public API surface, so by definition many things are potentially breaking. Some changes are tougher to stomach than others, but now that we have a definition for what breaking changes are:

https://microsoft.github.io/MixedRealityToolkit-Unity/Documentation/Contributing/BreakingChanges.html

We need to have some proposal and thoughts for how we can lessen the impact of those sorts of changes on consumers.

wiwei commented 5 years ago

We have a few folks who are looking at this @MenelvagorMilsom @thalbern

I'm going to use this issue to track both new breaking changes which this work will need to accomodate, and also learnings from attempts here.

Edit: Will no longer be using this to track new breaking changes, as this is conflating a few different concepts

wiwei commented 5 years ago

One thought I had was around adding a migration window to the MRTK itself, which, for things like profiles, could grep through your Assets and look for older profiles and then update them.

Basically, something like this:


using Microsoft.MixedReality.Toolkit.Editor;
using Microsoft.MixedReality.Toolkit.Input;
using Microsoft.MixedReality.Toolkit.Utilities;
using System;
using UnityEditor;

/// <summary>
/// A migration handler that will migrate user profiles based on the changes from
/// https://github.com/microsoft/MixedRealityToolkit-Unity/issues/4213
/// </summary>
class MeshJointVisualizationProfileMigrationHandler : BaseMigrationHandler
{
    public MeshJointVisualizationProfileMigrationHandler(string SearchRestrictionFilter = null) : base(SearchRestrictionFilter)
    {
        UpgradeFromVersion = new Version(2, 0, 0);
        UpgradeToVersion = new Version(2, 1, 0);
    }

    /// <inheritdoc />
    public override void Migrate()
    {
        var profiles = ScriptableObjectExtensions.GetAllInstances<MixedRealityHandTrackingProfile>();
        foreach (var profile in profiles)
        {
            MigrateProfile(profile);
        }
    }

    private void MigrateProfile(MixedRealityHandTrackingProfile profile)
    {
        SerializedObject serializedObject = new SerializedObject(profile);
        SerializedProperty enableHandMeshVisualization = serializedObject.FindProperty("enableHandMeshVisualization");
        if (enableHandMeshVisualization != null)
        {
            SupportedApplicationModes modes = enableHandMeshVisualization.boolValue ? SupportedApplicationModes.Editor | SupportedApplicationModes.Player : 0;
            SerializedProperty handMeshVisualizationModes = serializedObject.FindProperty("handMeshVisualizationModes");
            handMeshVisualizationModes.intValue = (int) modes;
            enableHandMeshVisualization.DeleteCommand();
            serializedObject.ApplyModifiedProperties();
        }
    }
}

And the general interface:


/// <summary>
/// An interface for migration handlers, which are used to migrate consumer code as they take new
/// versions of the MRTK.
/// </summary>
/// <remarks>
/// Each migration handler is designed to handle migrating from a specific version to another specific
/// version. Migration handlers can be chained (i.e. if there are breaking changes that actually
/// "depend" on each other, a migration handler from an older version will run before the migration
/// handler for a newer version)
/// </remarks>
abstract class BaseMigrationHandler
{
    /// <summary>
    /// If this is non-null, the underlying migration handler should use this to only migrate
    /// assets within this path.
    /// </summary>
    /// <remarks>
    /// This is used only for testing purposes.
    /// </remarks>
    protected string SearchRestrictionFilter;
    protected Version UpgradeFromVersion;
    protected Version UpgradeToVersion;

    public BaseMigrationHandler(string SearchRestrictionFilter = null)
    {
        this.SearchRestrictionFilter = SearchRestrictionFilter;
    }

    /// <summary>
    /// The version that the migration handler can migrate from. This handler will only be run if
    /// the version that the project will be migrated to is greater than the value returned from
    /// this function.
    /// </summary>
    /// <remarks>
    /// For example, if GetUpgradeFromVersion() returns 2.1.0, and the user is migrating from 2.0.1
    /// to 2.2.0, this migration handler will run.
    /// </remarks>
    public Version GetUpgradeFromVersion()
    {
        return UpgradeFromVersion;
    }

    /// <summary>
    /// The version that the migration handler can migrate to.
    /// </summary>
    /// <remarks>
    /// This is primarily used to sort migration handlers, so that handlers can be chained during
    /// longer-numbered upgrades. For example, when upgrading from 2.0.0 to 2.2.0, if there was
    /// a breaking change that occured from 2.0.0 to 2.1.0 and then another in the same file
    /// that went in 2.1.0 to 2.2.0, the two migration handlers would be run in the where the
    /// [2.0.0 -> 2.1.0] handler would run, then [2.1.0 -> 2.2.0] handler would run.
    /// </remarks>
    public Version GetUpgradeToVersion()
    {
        return UpgradeToVersion;
    }

    /// <summary>
    /// Runs the migration
    /// </summary>
    public abstract void Migrate();
}

After trying this out, there was one main problem with this approach, which is, in order for SerializedProperty to work (i.e. in order for SerializedObject to be able to find the property), the property still had to exist on the SerializedAsset. This means if we say, had this scenario:

1) Old profile has a property called "bool oldProperty" 2) New profile has a property called "FlagValue interestingFlagProperty" 3) There exists a mapping between the old and the new, where the new simply takes on all flag values if the old is true, and no flag values if the old is false.

In order for the migration handler to work in this case using built in Unity serialization APIs, the old property has to keep existing - otherwise SerializedObject.FindProperty won't find it. We COULD keep around old values to make this form of migration handler work... but it feels wrong to carry ongoing technical baggage (essentially forever) in order to make this in-editor experience work.

I think that instead we should have a similar setup but using powershell or python (whatever has a better YAML library)

wiwei commented 5 years ago

Calling out that a migration handler is needed for this change:

https://github.com/microsoft/MixedRealityToolkit-Unity/pull/5797

MenelvagorMilsom commented 5 years ago

So I began trying this in Python (PyYAML) yesterday evening and this morning. Here are some notes so far:

Reading Unity's YAML files is a little more awkward than expected.

From what I can see %TAG shorthands in YAML are valid per --- document and not per file, but Unity files have only one %TAG at the top of the file. This causes errors with PyYAML. This should be ok however, we don't need any information from the tags so this line and all !u![0-9]* strings from the start of --- documents.
PyYAML wasn't capturing the fileID specified at the beginning of every --- document. This is needed in order to reference existing objects and fields. This can be amended by converting each --- document in the file into a YAML object where the name is _<fileID>. This way, we only have one document in the YAML string. At the moment I've stripped away some information that I'll need in order to reconstruct the file, but it should be simple to add it in a way that doesn't interfere.

Both of these fixes can be done in a single pass over the document. Once these changes have been made to the YAML string, we should be in a position to make the changes we need.

Before After

Before	After
`%YAML 1.1 %TAG !u! tag:unity3d.com,2011: --- !u!29 &1 OcclusionCullingSettings: m_ObjectHideFlags: 0 serializedVersion: 2 m_OcclusionBakeSettings: smallestOccluder: 5 smallestHole: 0.25 backfaceThreshold: 100 m_SceneGUID: 00000000000000000000000000000000 m_OcclusionCullingData: {fileID: 0}`	`%YAML 1.1 --- _1: OcclusionCullingSettings: m_ObjectHideFlags: 0 serializedVersion: 2 m_OcclusionBakeSettings: smallestOccluder: 5 smallestHole: 0.25 backfaceThreshold: 100 m_SceneGUID: 00000000000000000000000000000000 m_OcclusionCullingData: {fileID: 0}`

%YAML 1.1
%TAG !u! tag:unity3d.com,2011:
--- !u!29 &1
OcclusionCullingSettings:
m_ObjectHideFlags: 0
serializedVersion: 2
m_OcclusionBakeSettings:
smallestOccluder: 5
smallestHole: 0.25
backfaceThreshold: 100
m_SceneGUID: 00000000000000000000000000000000
m_OcclusionCullingData: {fileID: 0}

%YAML 1.1
---
_1:
OcclusionCullingSettings:
m_ObjectHideFlags: 0
serializedVersion: 2
m_OcclusionBakeSettings:
smallestOccluder: 5
smallestHole: 0.25
backfaceThreshold: 100
m_SceneGUID: 00000000000000000000000000000000
m_OcclusionCullingData: {fileID: 0}

I also investigated unityparser, it looked promising, but doesn't work for scenes.

I was thinking we could define our patches in YAML files too. As I see it, there are only six possible changes to Unity YAML files that we'll need to make. Here's an example of a patch for a BoundingBox change I've made:

--- # Patch for moving min/max scaling from BoundingBox to TransformScaleHandler
versions:
from: 2.0.0
to: 2.1.0
components:
- BoundingBox: 23f9be7a3cb53794ead8b55e784b65aa
- TransformScaleHandler: 820f94d57893bf843a0cd6d93313fe6d
actions:
addField: []
deleteField: 
- component: BoundingBox
  name: scaleMinimum
- component: BoundingBox    
  name: scaleMaximum
renameField: []
modifyField: []
addComponent: 
- conditions:
    has: [BoundingBox]
    doesNotHave: [TransformScaleHandler]
  component:
    unityFields: {}
    componentFields:
      targetTransform: '{{{ MonoBehaviour<BoundingBox>::targetObject::Transform }}}'
      scaleMinimum: '{{{ MonoBehaviour<BoundingBox>::scaleMinimum }}}'
      scaleMaximum: '{{{ MonoBehaviour<BoundingBox>::scaleMaximum }}}'
      relativeToInitialState: 1
deleteComponent: []

As you can see, we need some method of referencing existing fields. That's what I'm going to attempt today.

MenelvagorMilsom commented 5 years ago

@wiwei an update:

Referencing existing values

For my test script, I have defined a simple syntax for referencing existing values in the scene. Here's an example:

addField:
    - component: BoundingBox
      name: test
      value: '{{{ MonoBehaviour<BoundingBox>::targetObject::Transform::m_LocalScale::x }}}'

This is an action (see patch syntax in previous comment). The value can be a raw value, or one of these reference strings, marked by triple curly braces. This is the current working syntax:

The links in the search are read left to right and are delimited by double colon ::.
The search will attempt to find the current link in the current object:
- If current object is a fileID, current object will be set to the object for that fileID before we do the following steps.
- If current link is found in current object it will set current object to be the object under current link.
- If not found and current object is a GameObject, search the current objects components for a object that contains current link.
The initial current object is always the GameObject of the component being modified.
MonoBehaviours require additional information to be found. The script type of the MonoBehaviour should be provided in angle brackets (e.g. MonoBehaviour\<BoundingBox>).
If the final link ends on a GameObject or Component, set current object to be it's fileID.

So if we look at the sample string above:

MonoBehaviour\<BoundingBox>: Not found in the initial GameObject, but it is found in the objects components, current object set to this MonoBehaviour object.
targetObject: Found in current object. Set current object to value at targetObject.
Transform: Current object is a fileID, so we reset current object to be the object at this fileID (in this case, it happens to be a GameObject). We then try to find Transform and can't, but we do find it in the GameObjects components. Set current object to be this Transform component.
m_LocalScale: Found in current object. Set current object to value at m_LocalScale.
x: Found in current object. Set current object to value at x.

So I think I have most of this working; I have yet to implement a few things. There currently no way to find a value in an array. Also, if objects in this chain are part of a prefab, things start falling apart. I can look into these things soon, but just wanted some feedback on this approach.

If we're able to reference existing values in the UnityAsset will the 6 actions I gave in my comment yesterday be enough to specify a patch?

Is Python the right choice?

So Python has been great for prototyping this, but is it desirable to add a Python dependency for migration? Is it alright to expect users to install Python just to upgrade their projects?

Also, are these migration tools something that we want to add testing for? If so that adds a dependency on Python for our build agents.

I don't necessarily think these are issues, but I thought it was worth chatting with you about it before committing too much to Python.

wiwei commented 5 years ago

@julenka for the python question as well.

A few loose thoughts here:

For python, I know that a lot of our scripts are powershell today (probably to work well with our general Azure DevOps system). The other thing here is that python is still probably more generally available across the different platforms that we support (i.e. mac, linux, windows), so even though it's an extra step I feel like it's not too onerous of an extra step. The alternative I guess is that we'd have to do things in powershell AND bash (which feels not great). For a dependency on our build agents, I feel like that also feels fine - again it's a fairly common scripting language that gets a lot of use in many different places, and even though it's another one I do feel like it's widespread and widely understood anyway.

So I think I have most of this working; I have yet to implement a few things. There currently no way to find a value in an array. Also, if objects in this chain are part of a prefab, things start falling apart. I can look into these things soon, but just wanted some feedback on this approach. If we're able to reference existing values in the UnityAsset will the 6 actions I gave in my comment yesterday be enough to specify a patch?

I think that this approach makes sense to me overall - I do have a question about some other migration scenario - in the case of https://github.com/microsoft/MixedRealityToolkit-Unity/pull/5797/files, this should work, but I can imagine that if we wanted to expand this to cover per-platform editor/player, this would probably require more work (i.e. if flag contains player, add flags windows/mac/linux player flags, if flag contains editor, add windows/mac/linux editor flags). To this it depends on where conditions: can be defined.

To that, it might also be interesting to have support for a "custom" handler (i.e. "if the conditions are met, call this handler script" - wherein the handler script could do more custom work and take in a before and then give an after YAML blob). I think that we may have more complicated migration scenarios that can come up, where having a handler run may take away the need to add more complex syntax/migration schema.

Generally though this approach sounds good to me - I think that if you're going to be writing up this stuff, I don't know if doing ALL of that and having it fully written is necessary (i.e. just adding support for the things you need now, and then setting the vision and direction for how folks can augment it it in the future to do that they need to do). I think right now we don't yet have the full picture of every single migration needed and the complexities that they have, so it would probably be better to do a minimal set of work here to get things going.

MenelvagorMilsom commented 5 years ago

@wiwei I've made some progress here. It's still not working in it's entirety, but a lot of the pieces are in place. Here are some more info:

Prefabs make everything harder

What I have written works for unmodified prefabs in theory, but as of yet I have not tackled prefab instance modifications. It should be possible to do, but I wanted to get other things working first. This is neither working for modifying prefab instance modification values or for accessing the values of prefab instance modifications in reference strings described above.

On top of this, Unity treats 3d model files (.fbx, .dae, .3ds, .dxf, .obj) as prefabs in it's own asset docs. As these files are not formatted like prefabs however (some are binary formats), parsing them is an issue. Unity appears to build additional metadata for these files so that they can be treated as prefabs, but I have yet to decipher how this works.

Dump results

Initially I was using PyYAML to parse yaml files, but it appears as if this module is no longer being actively developed. It was also problematic when you would dump the results. There was no way to maintain the existing format of the document. As a result, the Unity files would contain large unnecessary diffs.

So I switched to ruamel.yaml. Not only is this module actively developed, it also has a roundtrip loader/dumper, which maintains the formatting of the original document. Unfortunately, it has some of it's own quirks:

It interprets the guid 0000000000000000e000000000000000 (which I think is used for some default mesh in Unity) as a float, and so mangles the value when dumping.
It interprets the single character y as the boolean value true, wherever it occurs. This means, for example, a serialized vector2 {x: 0, y: 1} becomes {x: 0, true: 1}.

We could potentially raise these issues with the creators of this module. On the other hand, we could just modify these values during our initial pass of the document.

As well as this, you can speed up load/dump if you use the CLoader and CDumper, but you cannot use these and also RoundTripLoader and RoundTripDumper, which means that maintaining the original format comes at a price.

Code

WIP patching scripts are on my fork. If you want to have a look or give them a try, note that there's a lot still not working.

lukastoenneMS commented 5 years ago

5853 makes a few breaking changes wrt. settings in the input simulation profile. Some of them can be simply FormerlySerializedAs, but there are a number of non-trivial changes too.

In particular, all plain KeyCode and mouse button bindings in the profile have been replaced with a generic KeyBinding struct, which stores the type of binding (key or mouse) as well as the actual binding code (KeyCode or mouse button number respectively). The struct has its own inspector, which allows unified display and offers an "auto-bind" tool to quickly set key bindings by pressing the respective key instead of selecting from a huge dropdown list.

wiwei commented 5 years ago

I'm realizing now that tracking two different things in this thread doesn't make that much sense. I'm going to open up a new issue to track those and relink everything together.

Troy-Ferrell commented 5 years ago

FYI, #5967 is a candidate for inclusion in an auto-migration tool.

Atm, there are code paths in editor/runtime code to auto-migrate (runtime does not save changes though).

MenelvagorMilsom commented 4 years ago

It looks like unityparser got an update in October. I may investigate at some stage soon.

IssueSyncBot commented 9 months ago

We appreciate your feedback and thank you for reporting this issue.

Microsoft Mixed Reality Toolkit version 2 (MRTK2) is currently in limited support. This means that Microsoft is only fixing high priority security issues. Unfortunately, this issue does not meet the necessary priority and will be closed. If you strongly feel that this issue deserves more attention, please open a new issue and explain why it is important.

Microsoft recommends that all new HoloLens 2 Unity applications use MRTK3 instead of MRTK2.

Please note that MRTK3 was released in August 2023. It features an all new architecture for developing rich mixed reality experiences and has a minimum requirement of Unity 2021.3 LTS. For more information about MRTK3, please visithttps://www.mixedrealitytoolkit.org.

Thank you for your continued support of the Mixed Reality Toolkit!

microsoft / MixedRealityToolkit-Unity

Migration and breaking changes mitigations #5772

Referencing existing values

Is Python the right choice?

Prefabs make everything harder

Dump results

Code

5853 makes a few breaking changes wrt. settings in the input simulation profile. Some of them can be simply FormerlySerializedAs, but there are a number of non-trivial changes too.