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

Watch all files referenced by glTF asset #167

Merged
merged 2 commits into from
Nov 25, 2019

Conversation

bghgary
Copy link
Contributor

@bghgary bghgary commented Jun 9, 2019

See #163 (comment).

This works, but for textures, it seems to only update for Babylon. Not sure why, but Babylon somehow makes that the other two engines want to get the new texture also. Maybe it's some kind of caching going on, but I couldn't figure it out.

@najadojo
Copy link
Contributor

It's been a while since I looked at the extension but can you do the watching in the editor's context (to solve the issue)? I assume the watcher is running with the preview in a separate webview.

@bghgary
Copy link
Contributor Author

bghgary commented Jun 10, 2019

The watcher is already running in the editor context and is posting a refresh message to the preview panel webview when something changes.

The issue isn't about watching files. Watching files work just fine. Once the refresh message is sent to the webview, if the engine is running Cesium or Three.js, the engine doesn't pick up the update to the files for some reason.

@bghgary
Copy link
Contributor Author

bghgary commented Jun 10, 2019

Specifically, it only doesn't work for textures. It works fine for glTF files. I haven't tested changing the bin files, but I suspect it works because I can see in the network tab of the dev tools that it's trying to get the bin files. The same does not happen for image files.

@bghgary
Copy link
Contributor Author

bghgary commented Aug 19, 2019

@emackey Do you think it's okay to merge this without it working perfectly on all the engines? I don't have time to figure this out and I don't want this PR to just stay here.

@emackey
Copy link
Member

emackey commented Aug 21, 2019

@bghgary I'll have to carve out some time to work on this extension again.

Also I've seen a small issue (in master, in the published version), where the first change to a glTF file triggers a refresh before the "dirty" flag gets set on the document. So, if I'm previewing a glTF and I've moved the camera somewhere interesting, my very first keystroke in the glTF document causes a preview refresh and loss of camera position, and then after that it knows from the dirty flag to stop sending refreshes with subsequent keystrokes. I'm concerned that it's a little trigger-happy.

But that's separate from this PR. Should this PR come out of draft mode?

@bghgary
Copy link
Contributor Author

bghgary commented Aug 21, 2019

But that's separate from this PR

+1

Should this PR come out of draft mode?

If you're okay with merging it as it is without fixing all the engines, then I will make it ready.

@bghgary bghgary marked this pull request as ready for review August 21, 2019 22:38
@emackey
Copy link
Member

emackey commented Nov 25, 2019

I found an issue:

  1. Preview a glTF model in Babylon.
  2. Close the preview window.
  3. Overwrite a texture file used by the model.

It posts a refresh to a disposed webview. I can see code that suggests it's not supposed to be doing that, but currently this throws an unhandled exception.

@bghgary
Copy link
Contributor Author

bghgary commented Nov 25, 2019

Good catch! Fixed.

@emackey emackey merged commit 7b1e7b0 into AnalyticalGraphicsInc:master Nov 25, 2019
@bghgary bghgary deleted the preview-update-fix branch November 25, 2019 22:32
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.

None yet

3 participants