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

Normals issues with imported skinned model #206

Closed
hybridherbst opened this issue Sep 14, 2020 · 10 comments
Closed

Normals issues with imported skinned model #206

hybridherbst opened this issue Sep 14, 2020 · 10 comments
Labels

Comments

@hybridherbst
Copy link
Contributor

hybridherbst commented Sep 14, 2020

While importing the Apple Event usdz file, I noticed that the imported normals are broken / flipped at a certain point during the animation. This results in incorrect lighting and artifacts during the animation.

200914-131613

Note that there's a visible "flip" during the animation, and there's a noticeable seam in the normals:
200914-131828

Model link: https://github.com/Unity-Technologies/usd-unity-sdk/files/5218033/apple_event.usdz.zip

@jcowles
Copy link
Contributor

jcowles commented Sep 21, 2020

Seems this model is a great stress test for corner cases -- we'll take a look.

Thanks for reporting!

@jcowles jcowles added the bug label Sep 21, 2020
@jcowles
Copy link
Contributor

jcowles commented Sep 21, 2020

Are you sure this is a problem with normals? It looks like the real problem is we are not correctly importing the emissive color (at least for the built-in pipeline).

In the image below, the left side was the default import behavior and the right side I set manually by looking at the source USD file:

image

@jcowles
Copy link
Contributor

jcowles commented Sep 21, 2020

This is what it looks like with my local emission fix (the color banding is coming from gif compression):

emission-fix

@jcowles
Copy link
Contributor

jcowles commented Sep 21, 2020

@hybridherbst if you feel like there are additional bugs here, please reopen!

@jcowles jcowles closed this as completed Sep 21, 2020
@jcowles jcowles reopened this Sep 21, 2020
@jcowles
Copy link
Contributor

jcowles commented Sep 21, 2020

Upon further inspection, I think there really are two different bugs here -- or at least, we should look into why the normals are funky (they may just be authored that way in the USD file)

@jcowles
Copy link
Contributor

jcowles commented Sep 21, 2020

It looks like the authored normals are constant, which would only look correct in the rest position (note that this mesh is deformed by UsdSkel).

If I request Unity to recompute the normals, via advanced settings, they look correct.

@hybridherbst can you confirm?

@hybridherbst
Copy link
Contributor Author

hybridherbst commented Sep 21, 2020

With "constant" you mean the mesh doesn't have any normals? Otherwise, shouldn't using UsdSkel also correctly transform them? Recomputing normals breaks continuity in most cases, and obviously removes the user-authored normals.

By the way, another model that exhibits a very similar looking normal deformation (without animations) on Unity side when importing is the Gramophone from the QuickLook gallery:
https://developer.apple.com/augmented-reality/quick-look/
If you import it it has a similar-looking normal rotation/flip thing going on near the neck that does not look that way in ARQL or other softwares.
image

jcowles added a commit that referenced this issue Oct 1, 2020
judubu added a commit that referenced this issue Dec 21, 2020
* Remove the "rebuildAll on clone" logic and add guid as mesh names. (FTV-244)

We used to force a rebuild of the hierarchy when a UsdAsset (game object)
was duplicated to avoid sharing meshes and materials.
This caused an issue when game objects created from usd prefabs in
PlayMode where flagged for rebuild, losing all local edits.

I decided to remove the "rebuildAll on clone" logic and keep meshes and
materials shared between copies of UsdAsset imported as game objects.
That's the behavior I expect when I duplicate a usd asset reading the same
location in a usd file. It's also safe to do for skinned meshes as skinning is applied
in a vertex shader and the underlying shared mesh is not deformed.

The only drawback is that it won't work on deformed meshes in the case
you change playback speed between the copies. But the solution is simple,
you just have to reload and rebuild the hierachy in the UsdAsset.

I also set the name of the meshes with a guid to make it obvious that
the meshes are shared.

* Mesh names contains the go name followed by a short guid

* Only mark a mesh as dynamic when it is animated

* [HOTFIX] Fix game object hierarchy when loading as GameObject

The previous change was bypassing the call to UsdMenu.GetDefaultRoot,
creating broken hierarchies using the absolute root of the usd file "/".

(cherry picked from commit 8d9ffd0)

* Rename master to stable in yamato scripts

* TypeBinder: Add option to disable JIT for bindings

* Scene: Remove BG Executor & Async IO

* Logging: Refactor diagnostic delegate

* IL2CPP: Add "Preserve" attribute

* Swig: Add IL2CPP callback support

For IL2CPP to handle callbacks correctly, the must be tagged with MonoPInvokeCallback

* Swig: Fix order of VtValue/VtArray includes

* Tests: Add unit test for IL2CPP Attributes

* Regenerate code

* BUILDING: Update build steps

* Add Linux library

* Add OSX library

* OSX Build: New build with RPATH

* Docs: Add OSX build notes to BUILDING.md

* export texture value scale (e.g. color * texture)

* test data and test scene

* sample data root gameobject for easier testing

* time-to-frames conversion to prevent QuickLook bugs

* added sample data

* coding style

* requested changes

* UPM-CI: update registry path from bintray to artifactory

* fix texture export pathes for exporting from packages

* support texture wrap modes

* wrap mode test data

* move wrapmode to TextureReaderSample

* USD.NET: Update libs

* ShaderExport: Fix merge conflict fallout

* enabled USDZ export from recorder clip with tmp file (matches UsdzExporter logic)

* fixes to USDZ in-memory export, still WIP

* correct scale when exporting USDZ from timeline clip

* added sample data

* cleanup of temp directory handling and naming

* moved USDZ scale into ExportContext

* Tests: Fix meta file name case problem

* Conventions: fix braces

* CI: switch utils to stable to match the stable VM image

* Timeline: Fix playable name

* first rough pass of exporting in-memory textures and render textures, plus some sane handling of failure cases

* random naming of textures to prevent accidental overrides - TODO needs better system that acutally prevents collisions

* texture name uses original texture name now if it's in AssetDatabase (random name if dynamic texture)

* allow exporting alpha textures

* sample data added

* formatting and cleanup

* requested changes

* add comment for random naming. Also remove "simplification" if texture is in AssetDB (would lead to collisions with textures with identical names)

* TextureExport: Clarify names and clean up conventions

* fix animation export end time regression

* remove duplicate lines

* Fix: load scope as xform (FTV-417)

#154

Scopes are not Xformable per se but can contain payload and hold variantSets. If we want to be able to set the variant before loading the payload we need to load the Scope as a xform.

* Add comment explaining why Scopes are not added to the PrimMap

* Add tests

* Update Usd.Tests asmdef

* Fix test data path

* Add more tests

* Fix tests assertions

* Revert "Update Usd.Tests asmdef"

This reverts commit 4053d6c

* CI: use package-ci/mac image instead of buildfarm/mac

Fix the following errors:
```
[12:31:47.729 INF] UPM-CI is not compatible with your installed version of Nodejs (v9.8.0). Please use Nodejs v10 or above.
For more information on how to resolve this error, including in CI: https://github.cds.internal.unity3d.com/unity/upm-ci-utils#upm-ci-is-not-compatible
---------
/usr/local/lib/node_modules/upm-ci-utils/scripts/postinstall.js:9
    throw new Error(`Incompatible Nodejs Version (${process.version})`);
    ^

Error: Incompatible Nodejs Version (v9.8.0)
    at checkEngineVersion (/usr/local/lib/node_modules/upm-ci-utils/scripts/postinstall.js:9:11)
    at Object.<anonymous> (/usr/local/lib/node_modules/upm-ci-utils/scripts/postinstall.js:13:1)
    at Module._compile (module.js:649:30)
    at Object.Module._extensions..js (module.js:660:10)
    at Module.load (module.js:561:32)
    at tryModuleLoad (module.js:501:12)
    at Function.Module._load (module.js:493:3)
    at Function.Module.runMain (module.js:690:10)
    at startup (bootstrap_node.js:194:16)
    at bootstrap_node.js:666:3
```

* CI: updating promotion.yml with package-ci.mac image

* added isSRGB support

* added conversion type option to SetupTexture

* add super rough PBR support by using shaders from GLTF repo directly

* remove debug log and clean up code

* ChannelCombiner now supports arbitrary channel combinations and inversions

* added DamagedHelmet + MultiMaterial examples

Damaged helmet is open source, MultiMaterial is from me and I'll opensource it

* sample scene

* Shading: Fix duplicate shader parameter bug

* HDRP: Fix emissive map export

* added HDRP PBR Test assets

* fix transparency issues with HDRP export, add keyword checks for common features

* reduce PBR test set size from 65MB to 13MB

* starting to fix specular/metallic for HDRP export

* add support for exporting textures from disk inside package folders (2019.2+)

* added foundations for normal conversion

* added bumped cubes test data for normals export

* re-add rough normal conversion shader

* normals export from memory should work now, not 100% sure about colorspaces for other exports.

* formatting, correct sRGB setting (off) for data textures

* proper normal conversion for Standard / Built-in render path

* added AO-Planes test

* add conversion method for MaskMap to ORM

* restore helmet normal map (was test map)

* USD 20.08: Update BUILDING.md

* USD 20.08: Update minimal build command

* USD 20.08: Update Python build scripts for Python3

* USD 20.08: Update Swig and third_party headers

* USD 20.08: Add property API exports for UsdCs.dll

* USD 20.08: Upgrade UsdCs to VS2019, Win10

* USD 20.08: Fix assembly info file

* USD 20.08:  Regenerate bindings & libs

* USD 20.08: Fix MaterialSample required by API changes

* USD 20.08: Update USD libs for Windows

* USD 20.08: Update libs for final USD 20.08 build

* CI: Update UPM to run on 2019.4, 2020.1

* Serialization: Use CLS compliant IntPtr conversion

* USD 20.08: Update DLLs after CLS change

* USD v20.08: Build UsdCs for Linux

* USD 20.08: Update initUsd for new plug info path

* USD 20.08: Disable xcode code signing

* USD 20.08: Update OSX libs

* USD 20.08: Update USD version in package manifest

* USD 20.08: Update package version to 1.0.4

* USD 20.08: Update Changelog

* USD 20.08: Clean up share folder in bundle

* USD 20.08: Update OSX libs with correct versions

Alembic version was incorrect.
Updated dependent libs for good measure.

* USD 20.08: Remove unused plugInfo files

* USD 20.08: Remove OSX-specific pluginfo files

These used to be required because .dylib was burned into them, however this is no longer the case.

* USD 20.08: Update initUsd to share common plugInfo path for OSX

* ShaderImport: Default emissive color to white

Fixes #206

* ShaderImport: Emissive fixes for HDRP and light baking

* Package: Remove unused meta file

* Udate version in package.json and bump minimum unity version

* Udate changelog for 1.0.3-preview.2

* Yamato: Convert dummy jobs into proper virtual jobs

* Editor: Remove experimental namespace in 2020.2

* Create PR template based on the one used on Ono

* add null check for scene.AccessMask (USDU-13 #215)

* Maintain material names on import (USDU-91 #213)

* set material name to material path

* add unit test

* update changelog

* use name instead of full path

* Initialize USDPayloads.m_isLoaded with its prim load state (USDU-123) (#214)

* Initialize USDPayloads.m_isLoaded with the USD prim state rather than the policy. Otherwise it won't update accordingly if we load/unload the payloads.

* Better naming for tests class and one of the test.

* Update Changelog

* Simplified check on prim

* Clean up tests

* Fix UsdAsset not dirty (USDU-131) (#218)

* Dirty USDAsset in simple view when there's a change. Otherwise, modifications would be lost.

* Update Changelog

* Fix corrupted scene primMap with invalid prim (USDU-60) (#158, #220)

* Fix the scene primMap that was corrupted with invalid prim (USDU-60)

Now, if user try to read a prim at a path that doesn't exist, the
invalid prim is simply ignored. So, when writting back the prim, it's
written as a new prim at the given path.

* Update CHANGELOG (USDU-60)

* Add tests to USD.NET tests (USDU-60)

* Remove test from unit tests (USDU-60)

Co-authored-by: Julien Dubuisson <julien.dubuisson@unity3d.com>

* Ensure stage are reloaded on Refresh and Reload actions (USDU-58) (#155, #216)

* Force stage reloading on Reload and Refresh action (USDU-58)

UsdStage.Open won't open again a layer that is already open. So if the
layer has changed on disk, it needs either to be closed and re-open or
simply reloaded to get the new changes. Otherwise, USD will keep the
opened layer chached it already has in memory.

* Update Changelog (USDU-58)

* Add Unit Tests (USDU-58)

* Let the editor tick to register testfile change (USDU-58)

* Revert unneeded asmdef changes (USDU-58)

* Fix tests by making sure files' mtimes get modified

* Move data file into appropriate folder

* Remove unsused variable

Co-authored-by: Julien Dubuisson <julien.dubuisson@unity3d.com>
Co-authored-by: Thomas Tu <thomastu@unity3d.com>

* Exporting transform overrides should export usda files (USDU-124) (#222)

* Export overrides: use fileExtension param (USDU-124)

* Update changelog (USDU-124)

* Add all usd extensions to the export dialog (hierarchy and overrides)

Co-authored-by: Julien Dubuisson <julien.dubuisson@unity3d.com>

* Left handed orientation not handled correctly (USDU-9)  (#223)

* check handedness of file before changing axis

* update changelog

* add unit test

* code review fixes

* fix withCamera fbx reference not working on all Unity versions

* code review fix - add parens around &&

* code review fix - use epsilon comparison with Vector3's

* Revert "update changelog"

This reverts commit 4d44add.

* revert changes to MeshImporter

* fix left handed cube tests

- fix tests to work with original import implementation

* remove unused using statement

* Create sparse timesamples (USDU-46) (#221)

* use the UsdUtilsSparseAttrValueWriter to write time samples

* Make sure the transform of the SkelRoot object is set

* add unit test

* update changelog

* disable batchmode when running playmode tests

* commit rebuilt dll

* store SparseValueWriter as member variable

- use SparseValueWriter instead of SparseAttrValueWriter so that map from attribute to SparseAttrValueWriter is stored and SparseAttrValueWriter is not recreated each time a value is set.

* update dll after merge

* Drop Unity 2018 support (#227)

* Samples maintenance (USDU-65 USDU-136) (#226)

* Add some path utilities (USDU-65)

Needed to remove the hard coded paths in the samples.
Tests to come.

* ImportMesh Sample: remove hardcoded path (USDU-65)

* ImportMesh Sample: fix compatibility error (USDU-136)

Sample scene was erroring when loaded in 2019.4 because of an non
longer available component

* Add relative path utilities (USDU-65)

And tests

* ImportMaterials Sample: remove hardcoded path

 (USDU-65)

* ImportMaterials Sample: fix compatibility error

(USDU-136)

* ImportMesh Sample: use relative path utils (USDU-65)

* ExportMesh Sample: fix compatibility error (USDU-136)

* Playable Sample: remove hardcoded path (USDU-65)

* HelloUSD Sample: fix compatibility error (USDU-136)

* Remove unused path functions (USDU-65)

* Playable Sample: add missing asmdef (USDU-65)

* Update the changelog with old PRs and github issue ids (USDU-138) (#224)

* Update the changelog with old PRs and github issue ids.

* Updating package manifest for unity min version and package version

* Add changelog from #170

* Update linux binary from CentOS 7 VM (#230)

* Add .editorconfig and reformat the whole package to Unity standards. (#229)

* Release 2.0.0 (#231)

* [HOTFIX] Fix game object hierarchy when loading as GameObject

The previous change was bypassing the call to UsdMenu.GetDefaultRoot,
creating broken hierarchies using the absolute root of the usd file "/".

* Rename master to stable in yamato scripts

* [HOTFIX] Fix game object hierarchy when loading as GameObject

The previous change was bypassing the call to UsdMenu.GetDefaultRoot,
creating broken hierarchies using the absolute root of the usd file "/".

(cherry picked from commit 8d9ffd0)

* Udate version in package.json and bump minimum unity version

* Udate changelog for 1.0.3-preview.2

* UPM-CI: update registry path from bintray to artifactory

* Update changelog with new version number and release date

* Fix name generation of exported in memory textures

* Move yamato pack job to win10 vm

* Fix post build action on Win and Linux

* Add post build action on OSX

* Remove legacy package-manager-ui package from TestProject

Co-authored-by: Marie Fetiveau <marief@unity3d.com>
Co-authored-by: Lucille Caillaud <lucille.caillaud@unity3d.com>

Co-authored-by: Jeremy Cowles <jeremy.cowles@gmail.com>
Co-authored-by: bokken <ju@unity3d.com>
Co-authored-by: bokken <bokken@Mac-mini.local>
Co-authored-by: Felix Herbst <herbst@prefrontalcortex.de>
Co-authored-by: vkovec <viktoria.kovecses@gmail.com>
Co-authored-by: thomas-tu <45040865+thomas-tu@users.noreply.github.com>
Co-authored-by: Lucille Caillaud <51753559+lucillecaillaud@users.noreply.github.com>
Co-authored-by: Thomas Tu <thomastu@unity3d.com>
Co-authored-by: Marie FETIVEAU <703797+mfe@users.noreply.github.com>
Co-authored-by: Marie Fetiveau <marief@unity3d.com>
Co-authored-by: Lucille Caillaud <lucille.caillaud@unity3d.com>
@vickycl
Copy link
Collaborator

vickycl commented Jul 25, 2023

This appears to be fixed, thank you @lee-aandrew for testing :)
github_issue_206_360

@vickycl vickycl closed this as completed Jul 25, 2023
@hybridherbst
Copy link
Contributor Author

Suspiciously small error messages in the gif but I can't read them :D Thanks!

@vickycl
Copy link
Collaborator

vickycl commented Jul 26, 2023

Yeah, unfortunately those are the errors from your other bug #205 which we haven't been able to address yet 😅

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

No branches or pull requests

3 participants