[BUG] Replaces the current Render pipeline!!

melMass commented 3 years ago

Describe the bug It was a bit dumb of me to no test it in isolation but I messed a project pretty badly just by installing Mixture through UPM (first time I use Open UPM), maybe add a note in the readme?

To Reproduce Steps to reproduce the behavior:

cd to your HDRP Unity Project
openupm add com.alelievr.mixture
See error:

WARN 404 package com.unity.render-pipelines.core@10.0.0-preview.27 is not a valid choice of...
WARN 404 fall back to com.unity.render-pipelines.core@9.0.0-preview.77

alelievr commented 3 years ago

Hello,

Sorry to hear that :( installing a package should not mess up a project, at worst you should have a compilation error, otherwise, it's an issue on the package manager side.

Which version of HDRP were you using? Currently, mixture is using the 10.1.0 and it doesn't compile with any previous versions, maybe that's the issue?

It could also be that the openupm add command did something weird to your project manifest. I think it's safer to install the package using the scoped registries in unity as I explain in my readme.

melMass commented 3 years ago

I think it's the later as all my shaders broke! But all seems normal in it!

I was using 9.0.0-preview.71 as my project is still using 2020.1 but once I manage to get back to a working state I'll update to 2020.2 and HDRP 10.1

I'll report here how it goes, I'm still debugging it right now :)

melMass commented 3 years ago

I have no idea what's going on, I've reinstalled the same HDRP version I was using previously

alelievr commented 3 years ago

Okay, so you were using HDRP 9 with 20.1 when you executed the openupm command. It should have displayed an error and exited as Mixture is only available for unity 20.2 and above, if I have the time, I'll file an issue on the open UPM repo about this as it's not safe at all. The openupm command doesn't seem to check for conflicting dependencies which is another issue :/

PS: I added a line about the render pipeline version needed for Mixture in the readme: https://github.com/alelievr/Mixture/commit/0bfd2268ebb9e2263a28757ab37627f9b2107605

melMass commented 3 years ago

Removing mixture and restarting the project a few times did recompile everything correctly! Lesson learned! I'll look more into Open UPM before relying on it 😆 ... And Mixture looks really nice, can't wait to update everything, and start playing with it.

Closing this issue.

Thanks

favoyang commented 3 years ago

Thanks for reporting the issue. I'd like to investigate what happens and leaves the improvement part to https://github.com/openupm/openupm-cli/issues/16.

First, openupm-cli won't do something too magic for the manifest.json. It just fetches dependencies and fill the right scopes info to the file and leave the rest to the PackMan (the Unity Package Manager) to resolve and install tarballs for you. During the process, openupm-cli may warn you if it finds certain dependencies are not matched with the requirements.

Example: manifest.json changes made by openupm-cli

+    "com.alelievr.mixture": "0.0.3"
   },
   "scopedRegistries": [
     {
       "name": "package.openupm.com",
       "url": "https://package.openupm.com",
       "scopes": [
+        "com.alelievr.mixture",
+        "com.alelievr.node-graph-processor",
         "com.openupm"
       ]
     }
   ]

If you follow this project's README to do it manually:

       "name": "package.openupm.com",
       "url": "https://package.openupm.com",
       "scopes": [
-        "com.openupm"
+        "com.openupm",
+        "com.alelievr"

The only difference is that openupm-cli is more accurate about the scopes.

Now let's see what else happens when installing the package via openupm-cli to Unity 2020.2.0b8.

$ openupm add com.alelievr.mixture
WARN 404 package com.unity.render-pipelines.core@10.0.0-preview.27 is not a valid choice of 0.1.21, 0.1.28, 0.1.27, 1.0.0-beta, 1.0.1-beta, 1.1.1-preview, 1.1.2-preview, 1.1.4-preview, 1.1.5-preview, 1.1.8-preview, 1.1.10-preview, 2.0.1-preview, 2.0.3-preview, 1.1.11-preview, 2.0.4-preview, 2.0.4-preview.1, 2.0.5-preview, 3.0.0-preview, 2.0.6-preview, 2.0.7-preview, 2.0.8-preview, 3.1.0-preview, 3.3.0-preview, 4.0.0-preview, 4.0.1-preview, 5.0.0-preview, 4.1.0-preview, 4.2.0-preview, 5.1.0, 4.3.0-preview, 5.2.0, 5.2.1, 5.2.2, 4.6.0-preview, 5.2.3, 4.8.0-preview, 4.9.0-preview, 5.3.1, 4.10.0-preview, 5.6.1, 5.7.2, 5.8.2, 5.9.0, 5.10.0, 5.13.0, 6.5.3, 6.5.2, 6.7.1, 5.16.1, 6.9.0, 7.0.0, 7.0.1, 6.9.1, 7.1.1, 7.1.2, 6.9.2, 7.1.5, 7.1.6, 7.1.7, 7.1.8, 7.2.0, 7.2.1, 8.0.1, 7.3.1, 9.0.0-preview.13, 8.1.0, 7.4.1, 9.0.0-preview.35, 9.0.0-preview.38, 8.2.0, 10.0.0-preview.30, 7.4.2, 7.4.3, 9.0.0-preview.60, 7.5.1, 10.1.0, 9.0.0-preview.77
WARN 404 fall back to com.unity.render-pipelines.core@9.0.0-preview.77
WARN 404 package not found: com.unity.ugui
notice manifest added com.alelievr.mixture@0.0.3
notice please open Unity project to apply changes

It warns you that com.unity.render-pipelines.core@10.0.0-preview.27 is not a valid version. It also warns about com.unity.ugui, but it's a special package. Let's focus on the first one.

List all versions of com.unity.render-pipelines.core below. It ordered by the package publish time. Because Unity needs to support old versions as well, their submission versions didn't always go up. There are lots of patch updates. That's why openupm-cli said, "fall back to com.unity.render-pipelines.core@9.0.0-preview.77". It may be a bit confusing that we won't actually install the "9.0.0-preview.77" for you, we just found the latest version is "9.0.0-preview.77". Unity's PackMan decides which version to install.

$ openupm view com.unity.render-pipelines.core
...
versions:
  9.0.0-preview.77
  10.1.0
  7.5.1
  9.0.0-preview.60
  7.4.3
  7.4.2
  10.0.0-preview.30
...

The only version starts with 10.0.0 is "10.0.0-preview.30". Is "10.0.0-preview.27" a typo? Otherwise, maybe Unity is unpublishing published versions (which can be rare and anti-practice).

The PackMan window tells that the package requires preview.27 and it already has the preview.30 installed.

If you repeat the process on Unity 2019.3. The PackMan refuses to install the entire package because of the missing dependencies. PackMan's logic is that if you already installed the unmatched dependency package manually, it will warn you. Otherwise, it will report you the missing dependencies and let you fill the missing pieces.

Packman didn't check the required unity version. But I get the point that openupm-cli could warn you that.

alelievr commented 3 years ago

Thanks for investigating this issue :)

I don't think the 10.0.0-preview.27 was a typo, it installed correctly for me. But I agree that it's a preview patch package and we shouldn't rely on it, it was just a temporary one made before the 10.1 release. Now that 10.1 is released I've updated the dependencies of Mixture to match this version.

And I totally agree that packman should check if the package is compatible with the current project setup before installing it.

A note about the WARN 404 fall back log openupm prints if it fails to find the correct package version. For me, this should be an error in this case because mixture depends on the 10.x version which means that it needs API that only exists in 10.x, not 9.x so the fallback version is not valid in this case. Because all the packages are supposed to use semvar, we can make this assumption: if the fallback package is using a different minor/patch version, then it's fine, but not a major version because there can be breaking change that breaks the compatibility.

favoyang commented 3 years ago

I don't think the 10.0.0-preview.27 was a typo, it installed correctly for me.

It's weird then. Taking of published version is not a good practice, even for a preview version. If you look at the published version below, you will find versions like "9.0.0-preview.38" and "9.0.0-preview.35", it's unlikely Unity is removing a published preview version when adding a new preview version. I guess it's a typo because the shader graph shares the same version 10.0.0-preview.27, but could be just a coincidence.

I will check this with Unity later.

versions:
  9.0.0-preview.77
  10.1.0
  7.5.1
  9.0.0-preview.60
  7.4.3
  7.4.2
  10.0.0-preview.30
  8.2.0
  9.0.0-preview.38
  9.0.0-preview.35
  7.4.1
  8.1.0
  9.0.0-preview.13
  7.3.1

And I totally agree that packman should check if the package is compatible with the current project setup before installing it.

Yes, it should check the min unity version.

A note about the WARN 404 fall back log openupm prints if it fails to find the correct package version. For me, this should be an error in this case because mixture depends on the 10.x version which means that it needs API that only exists in 10.x, not 9.x so the fallback version is not valid in this case. Because all the packages are supposed to use semvar, we can make this assumption: if the fallback package is using a different minor/patch version, then it's fine, but not a major version because there can be breaking change that breaks the compatibility.

It makes sense. But I want to point out two things,

The text should be improved, it doesn't means "fall back". It means openupm-cli finds a missing dependency version and warns you. Because the cli doesn't install packages directly, it only modifies manifest.json.
Packman's current behavior of handling a missing dependency version is that unless you install the dependency package manually (can be any version), it refuses to install the package for you. For example, because 10.0.0-preview.27 is not a valid version of com.unity.render-pipelines.core, packman shall refuse to install com.alelievr.mixture. However because you always has a version of com.unity.render-pipelines.core installed, packman still install com.alelievr.mixture for you.

It would be better if PackMan can understand the standard semver prefix: a tilde (~) and a caret (^). What you described is ^major.minor.patch - lock the major version only.

For now, I can just let it fail for missing a dependency or a dependency version, and then introduces a -f option for force install.

favoyang commented 3 years ago

Refs Unity forum thread of unpublishing preview version: https://forum.unity.com/threads/will-unity-take-off-a-published-preview-version.1005114/

favoyang commented 3 years ago

The changed behavior of openupm-cli add command: https://github.com/openupm/openupm-cli/issues/16#issuecomment-727600606

Examples

1) Try to install the mixture to a lower unity version

$ openupm add com.alelievr.mixture
WARN editor.version requires 2020.2.0a13 but found 2019.2.13f1
notice suggest upgrade the editor to 2020.2.0a13, or run with option -f to ignore the warning

2) Try to install the mixture to a qualified unity version. It fails because com.unity.render-pipelines.core@10.0.0-preview.27 is not a valid version. Then it prompts to either contact the author to fix it or find a replaceable version yourself. This matched the current Unity behaviour - it will try to install the exact required version or it will fail.

$ openupm add com.alelievr.mixture
WARN 404 package com.unity.render-pipelines.core@10.0.0-preview.27 is not a valid choice of 0.1.21, 0.1.28, 0.1.27, 1.0.0-beta, 1.0.1-beta, 1.1.1-preview, 1.1.2-preview, 1.1.4-preview, 1.1.5-preview, 1.1.8-preview, 1.1.10-preview, 2.0.1-preview, 2.0.3-preview, 1.1.11-preview, 2.0.4-preview, 2.0.4-preview.1, 2.0.5-preview, 3.0.0-preview, 2.0.6-preview, 2.0.7-preview, 2.0.8-preview, 3.1.0-preview, 3.3.0-preview, 4.0.0-preview, 4.0.1-preview, 5.0.0-preview, 4.1.0-preview, 4.2.0-preview, 5.1.0, 4.3.0-preview, 5.2.0, 5.2.1, 5.2.2, 4.6.0-preview, 5.2.3, 4.8.0-preview, 4.9.0-preview, 5.3.1, 4.10.0-preview, 5.6.1, 5.7.2, 5.8.2, 5.9.0, 5.10.0, 5.13.0, 6.5.3, 6.5.2, 6.7.1, 5.16.1, 6.9.0, 7.0.0, 7.0.1, 6.9.1, 7.1.1, 7.1.2, 6.9.2, 7.1.5, 7.1.6, 7.1.7, 7.1.8, 7.2.0, 7.2.1, 8.0.1, 7.3.1, 9.0.0-preview.13, 8.1.0, 7.4.1, 9.0.0-preview.35, 9.0.0-preview.38, 8.2.0, 10.0.0-preview.30, 7.4.2, 7.4.3, 9.0.0-preview.60, 7.5.1, 10.1.0, 9.0.0-preview.77, 10.2.0
notice suggest to install com.unity.render-pipelines.core@10.0.0-preview.27 or a replaceable version manually
ERR! missing dependencies please resolve thie issue or run with option -f to ignore the warning

3) Try to install a replaceable version

$ openupm add com.unity.render-pipelines.core@10.0.0-preview.30
notice manifest added com.unity.render-pipelines.core@10.0.0-preview.30
notice please open Unity project to apply changes

then retry install the mixture. It still warns about the missing dependency version issue, but it noticed you installed a replaceable version, and let you install the com.alelievr.mixture@0.0.3 package.

$ openupm add com.alelievr.mixture
WARN 404 package com.unity.render-pipelines.core@10.0.0-preview.27 is not a valid choice of 0.1.21, 0.1.28, 0.1.27, 1.0.0-beta, 1.0.1-beta, 1.1.1-preview, 1.1.2-preview, 1.1.4-preview, 1.1.5-preview, 1.1.8-preview, 1.1.10-preview, 2.0.1-preview, 2.0.3-preview, 1.1.11-preview, 2.0.4-preview, 2.0.4-preview.1, 2.0.5-preview, 3.0.0-preview, 2.0.6-preview, 2.0.7-preview, 2.0.8-preview, 3.1.0-preview, 3.3.0-preview, 4.0.0-preview, 4.0.1-preview, 5.0.0-preview, 4.1.0-preview, 4.2.0-preview, 5.1.0, 4.3.0-preview, 5.2.0, 5.2.1, 5.2.2, 4.6.0-preview, 5.2.3, 4.8.0-preview, 4.9.0-preview, 5.3.1, 4.10.0-preview, 5.6.1, 5.7.2, 5.8.2, 5.9.0, 5.10.0, 5.13.0, 6.5.3, 6.5.2, 6.7.1, 5.16.1, 6.9.0, 7.0.0, 7.0.1, 6.9.1, 7.1.1, 7.1.2, 6.9.2, 7.1.5, 7.1.6, 7.1.7, 7.1.8, 7.2.0, 7.2.1, 8.0.1, 7.3.1, 9.0.0-preview.13, 8.1.0, 7.4.1, 9.0.0-preview.35, 9.0.0-preview.38, 8.2.0, 10.0.0-preview.30, 7.4.2, 7.4.3, 9.0.0-preview.60, 7.5.1, 10.1.0, 9.0.0-preview.77, 10.2.0
notice manifest added com.alelievr.mixture@0.0.3
notice please open Unity project to apply changes

@alelievr thanks for fixing the dependencies issue, however, you still need to release a new Git tag to notify OpenUPM.

alelievr commented 3 years ago

Thanks for modifying the add behavior :) I'll do a new release right now

favoyang commented 3 years ago

@alelievr could you comment on this thread https://github.com/alelievr/Mixture/issues/17 ?

Just to make sure that Unity won't remove a published version (it seems they won't).

alelievr / Mixture

[BUG] Replaces the current Render pipeline!! #17