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

Meldis auto-reloading misses changes to glTF files #21194

Open
SeanCurtis-TRI opened this issue Mar 26, 2024 · 9 comments · May be fixed by #21300
Open

Meldis auto-reloading misses changes to glTF files #21194

SeanCurtis-TRI opened this issue Mar 26, 2024 · 9 comments · May be fixed by #21300
Assignees
Labels
component: geometry illustration What and how geometry gets communicated to external visualizers type: bug

Comments

@SeanCurtis-TRI
Copy link
Contributor

What happened?

Meldis's _GeometryFileHasher provides meldis the ability to recognize if a model has changed on disk since it was last loaded and intelligently reload (by considering the set of files that define the model). However, that logic is heavily obj-centric. Changes to glTF data on disk do not trigger a model reload.

Now that Meshcat (the Drake class, not the javascript library) has the intelligence to run down all linked assets, that should be exploited in _meldis.py so that the right set of assets are included in the hashing.

Version

No response

What operating system are you using?

No response

What installation option are you using?

No response

Relevant log output

No response

@SeanCurtis-TRI SeanCurtis-TRI added type: bug component: geometry illustration What and how geometry gets communicated to external visualizers labels Mar 26, 2024
@jwnimmer-tri
Copy link
Collaborator

Per slack, we might attack this in two stages:

(1) Fix the _GeometryFileHasher to know how to chase gltf uris.

(2) Refactor Meldis to lean on Meshcat::SetObject to report back exactly which files were relevant, so we can delete the bespoke Python logic.

@SeanCurtis-TRI
Copy link
Contributor Author

Stage one is in flight with #21297.

I think stage two requires a bit more discussion.

It's certainly reasonable to modify the return value of void Meshcat::SetObject(string_view, const Shape&, const Rgba&) to return a vector of paths. But what's unclear to me is how that benefits meldis.

Meldis would only find out what files were necessary after dispatching objects to meldis. Sure, it can save those paths and create a hash on what was already set into Meshcat. However, what good does that do when it gets the next load message? Knowing what's currently in meshcat doesn't help in interpreting the incoming message. If the only way to do that is set things into meshcat and ask what got set, hasn't it already done the work?

I can imagine the hasher creating its own Meshcat instance with a functionally non-communicative port so it can blindly register things and harvest the result. After all, it would really only ever need to register meshes, not everything in the message.

Alternatively, we can try to refactor the code in Meshcat so that the logic for assembling the family of files associated with a mesh is independent of what is done with that information? And then that core functionality can be used by a meshcat instance to "peek" but not "act".

Can you elaborate on what you had in mind?

@jwnimmer-tri
Copy link
Collaborator

If the new load message is the same as the old load message, and all of the files used in the prior load message still exist and are unchanged, then we know for certain that no reload is necessary. So all we need to do is keep track of the prior load message's overall list of files and their hashes (or maybe one overall hash), and that's all we need.

@SeanCurtis-TRI
Copy link
Contributor Author

SeanCurtis-TRI commented Apr 11, 2024

File existence should not be the arbiter of whether the load message matches. I can remove a reference to an image without deleting the image. Furthermore, I can add an image to a model and it won't be captured by what I previously loaded.

I'm leaning toward simply creating an API that takes a Mesh and returns a list of file info (at its simplest, just paths). That would also simplify what Meshcat does to handle objs. Ultimately, it would localize functionality that is currently distributed across Meshcat and FileStorage and make it accessible for anyone who is curious for whatever reason.

@jwnimmer-tri
Copy link
Collaborator

jwnimmer-tri commented Apr 11, 2024

If you remove the reference to an image (or add one), the checksum of the gltf (or whatever) that mentions that image will change.

@jwnimmer-tri
Copy link
Collaborator

I am pretty convinced that the right Meldis approach is "Meshcat tells us what files it touched" since that will very clearly be correct and never diverge as the code chnages, instead of having two different control paths where we hope that Mesh.tell_me_files() matches what Meshcat did. For all we know, Meshcat might eventually do things that go beyond what Mesh.tell_me_files() knows about, or ignore e.g. images that are for gltf extensionsUsed that are always ignored on the JS side that Mesh.tell_me_files() would need to return in general.

@SeanCurtis-TRI
Copy link
Contributor Author

Touche. I keep forgetting hashing the original file.

So, to summarize:

  • Viewer applet starts with no "model" of the loaded data (indicated by a None-valued self._hash value).
  • When a load message comes in:
    • If the applet has a model of a populated meshcat (self._hash is not None)
      • Create a current hash combining
        • every file directly mentioned in the load file.
        • every support file known to be used by meshcat (skipping those that cannot be loaded)
      • If the current hash matches self._hash, we're done
    • Perform the load work
      • While calling SetObject() collect up all of the files used.
        • Only store the supporting files (not the ones directly named in the message -- see above)
        • Any file in the new set of used files that wasn't in the old set needs to be added to the current hash.
        • Any file in the old set that isn't in the new set (but exists on disk) needs to be subtracted from the hash.
          • This is a problem, as I see it. We can't back bytes out of the hasher.
      • Store the current hash in self._hash

On the subject of "subtracting bytes from a hash value":

One possible solution is to replace a single aggregate hash value with a set of hash_values (on per file). Instead of comparing aggregate hash values, we compare sets of values. This would handle both cases where the set of files have changed (new files now being used and old files no longer used but not deleted).

@jwnimmer-tri jwnimmer-tri removed their assignment Apr 11, 2024
@jwnimmer-tri
Copy link
Collaborator

The flavor I imagined was that Meshcat.SetObject would return[1] the list of all relevant files, including the Mesh/Convex filename. I imagine that simplifies everything -- Meldis would not need to do anything with the load message's filename other than pass it to SetObject and then remember the list of files that comes back. We could promise the Mesh.filename() was first in the returned list, in case that helps anything. I'm not wedded to that though -- if for some reason treating the Mesh.filename() differently made the code cleaner then I'd be fine with that.

I'm open to the idea of multiple hash values, but my guess is that a single overall hash still ends up simpler. The "did any of these set of hashes change" versus "did the combined hash over this set of hashes change" seem equivalent. Again, though, I'm willing to trust however the simplest code comes out, whatever that looks like.

[1] ... or output argument, or return struct with named fields for future expansion, or ... etc

@SeanCurtis-TRI
Copy link
Contributor Author

I've simplified from what I previously wrote and will be submitting a PR soon. Ultimately, I'm trying to minimize the amount of hashing I'm doing. The goal is two fold:

  • Any hash we store should only depend on the actual files that got used this time and not include any files from the previous time.
  • Don't hash a file twice in repsonse to a load message.

One resolution, obviously, is to remove the second requirement. But that just seems inelegant to me. But we can move this conversation over to a concrete PR in a bit.

@jwnimmer-tri jwnimmer-tri self-assigned this Apr 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: geometry illustration What and how geometry gets communicated to external visualizers type: bug
Projects
Status: Todo
Development

Successfully merging a pull request may close this issue.

2 participants