BabylonJS / BabylonToolkit

Home of the community maintained Unity & Unreal exporters for Babylon.js
179 stars 47 forks source link

Babylon Toolkit breaks glTF loader in the playground #20

Closed bghgary closed 3 years ago

bghgary commented 3 years ago

https://github.com/BabylonJS/UnityExporter/blob/59f751da37f532fec7e20db97561d3c217f18268/Manager/src/babylon-toolkit.ts#L1078-L1088

I'm not sure what is going on with this code, but disposing the loader is causing issues for us in the playground. What is the reason for the dispose?

cc @sebavan @deltakosh

MackeyK24 commented 3 years ago

i think i saw somewhere to manually dispose the loader... But I can remove that

bghgary commented 3 years ago

Looking at this more, this whole event handler shouldn't be here. It's changing the loader properties for all glTF loaders that are instantiated. It will make the PG do bad things if BABYLON.SceneManager.AnimationStartMode is not null or BABYLON.SceneManager.ForceRightHanded is not null.

You should instead use Observable.addOnce and only do it right before you load a glTF asset.

MackeyK24 commented 3 years ago

I switched to addOnce... but it is only setting animationStateMode ONLY IF the BABYLON.SceneManager.AnimationStateMode != null

Same for coordinateSystemMode

These are ONLY NOT NULL when loading GLTF Content from UnityExport and the Animation Start Mode is Set

And its been running fine for over year now

MackeyK24 commented 3 years ago

BTW... @bghgary You may not remember, but you gave me that bit code snippet to change animationStartMode from not always starting up for unity content... And the way to force Right handed 👍

bghgary commented 3 years ago

Your code will work fine by itself. The problem is that this code is being loaded into the playground which can have other code running as well.

MackeyK24 commented 3 years ago

I get that its loaded on the playground, but it will not do anything unless you explicitly set BABYLON.SceneManager.AnimationStartMode or BABYLON.SceneMamanger.ForceRightHanded to a non null value.

But i will just remove it the whole BABYLON.SceneLoader.OnPluginActivatedObservable

bghgary commented 3 years ago

explicitly set BABYLON.SceneManager.AnimationStartMode or BABYLON.SceneMamanger.ForceRightHanded to a non null value

It's not likely to be a problem, but it would be better if you add this handler right before loading an asset and use addOnce. This will ensure the settings are correct.

At some point, I want to add a new way to set these options so that it's not so annoying to use.

MackeyK24 commented 3 years ago

Now uses ?UnityToolkit Option