Fix #114237: USD import crashes on reporting during cache handling. #114337

Bastien Montagne · 2023-10-31T19:38:15+01:00

Bastien Montagne commented

2023-10-31 19:38:15 +01:00

This usage of the whole USD USDStageReader as 'cache handle' feels a
bit... uncomfortable to me. It implicitely assumes that all data in this
owned by the reader (or has a lifetime that contains the whole handle
lifetime). IMHO, if such struct is expected to have a long lifetime, it
should be documented, and probably needs better design for sub-data
handling then. Am curious to know if there is a good reason why the
handle itself is not a simple trivial class, able to re-generate an
actual complete USD reader on demand?

Or maybe we need to find a better place to store the wmJobWorkerStatus
data?

Anyway, for now this PR addresses the issue by ensuring that the
worker_status stored in the USDImportParams is not nullptr before
trying to access it for the reports.

This is more of a short-term solution though, imho there should be a way
to pass a reportlist to this codepath, and ensure the wmJobWorkerStatus
is always valid?

This usage of the whole USD `USDStageReader` as 'cache handle' feels a bit... uncomfortable to me. It implicitely assumes that all data in this owned by the reader (or has a lifetime that contains the whole handle lifetime). IMHO, if such struct is expected to have a long lifetime, it should be documented, and probably needs better design for sub-data handling then. Am curious to know if there is a good reason why the handle itself is not a simple trivial class, able to re-generate an actual complete USD reader on demand? Or maybe we need to find a better place to store the `wmJobWorkerStatus` data? Anyway, for now this PR addresses the issue by ensuring that the `worker_status` stored in the `USDImportParams` is not `nullptr` before trying to access it for the reports. This is more of a short-term solution though, imho there should be a way to pass a reportlist to this codepath, and ensure the `wmJobWorkerStatus` is always valid?

Bastien Montagne added 1 commit 2023-10-31 19:38:28 +01:00

a8406abe2b Fix #114237 : USD import crashes on reporting during cache handling.

This usage of the whole USD `USDStageReader` as 'cache handle' feels a
bit... uncomfortable to me. It implicitely assumes that all data in this
owned by the reader (or has a lifetime that contains the whole handle
lifetime). IMHO, if such struct is expected to have a long lifetime, it
should be documented, and probably needs better design for sub-data
handling then. Am curious to know if there is a good reason why the
handle itself is not a simple trivial class, able to re-generate an
actual complete USD reader on demand?

Or maybe we need to find a better place to store the `wmJobWorkerStatus`
data?

Anyway, for now this PR addresses the issue by ensuring that the
`worker_status` stored in the `USDImportParams` is not `nullptr` before
trying to access it for the reports.

This is more of a short-term solution though, imho there should be a way
to pass a reportlist to this codepath, and ensure the `wmJobWorkerStatus`
is always valid?

Bastien Montagne requested review from Michael Kowalski 2023-10-31 19:38:47 +01:00

Bastien Montagne requested review from Brecht Van Lommel 2023-10-31 19:38:56 +01:00

Bastien Montagne commented

2023-10-31 19:41:34 +01:00

@brecht mostly added you here to have your view on the underlying topic (i.e. should a cache_file->handle keep a reference to a USDStageReader for its whole runtime lifetime, or should this handle rather be a lighter, simpler struct able to re-generate such a USDReader when needed?).

@brecht mostly added you here to have your view on the underlying topic (i.e. should a `cache_file->handle` keep a reference to a `USDStageReader` for its whole runtime lifetime, or should this handle rather be a lighter, simpler struct able to re-generate such a USDReader when needed?).

Michael Kowalski approved these changes 2023-10-31 20:31:02 +01:00

Michael Kowalski left a comment

This change looks good. Thank you for the fix!

I also see your main points and would be happy to discuss a re-design. The original implementation was based on the Alembic cache reader, of course, so a redesign would affect the Alembic import code path as well.

This change looks good. Thank you for the fix! I also see your main points and would be happy to discuss a re-design. The original implementation was based on the Alembic cache reader, of course, so a redesign would affect the Alembic import code path as well.

Brecht Van Lommel commented

2023-11-01 17:58:08 +01:00

I'm not familiar enough with this code to immediately know how it should be better structured.

I imagine it makes sense to keep around some state per prim/mesh to quickly do updates in animation playback with the mesh cache modifier. But every USD prim reader probably doesn't need to point back to things like the import params or object, that seems better to be passed in as a function argument when needed. And the import params also don't seem like the right place for a worker status, that also seems like something to pass in as a function argument.

I'm not familiar enough with this code to immediately know how it should be better structured. I imagine it makes sense to keep around some state per prim/mesh to quickly do updates in animation playback with the mesh cache modifier. But every USD prim reader probably doesn't need to point back to things like the import params or object, that seems better to be passed in as a function argument when needed. And the import params also don't seem like the right place for a worker status, that also seems like something to pass in as a function argument.

👍 1

Brecht Van Lommel approved these changes 2023-11-01 17:58:22 +01:00

Bastien Montagne merged commit 730b5057a7 into main

2023-11-02 12:45:37 +01:00

Bastien Montagne deleted branch tmp-fix-usd-crash-reports

2023-11-02 12:45:39 +01:00

Bastien Montagne referenced this issue from a commit

2023-11-02 12:45:41 +01:00

Fix #114237: USD import crashes on reporting during cache handling.

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Fix #114237: USD import crashes on reporting during cache handling. #114337