Load embedded geometry when an HDF5 agent file is selected - #14
Open
chraibi wants to merge 2 commits into
Open
Conversation
The bundled HDF5 is built without thread safety (Threadsafety: OFF in
libhdf5.settings), yet geometry and trajectory loading can read the same
.h5 concurrently from different threads. Worse, CloseFile(), OpenFile()'s
error path and DetectFormat() called H5close(), which shuts down the HDF5
library process-wide and crashes any reader that is mid-operation.
Observed as a SIGSEGV at address 0x20 when one thread ran H5close() while
another was still inside H5open():
Hdf5DataPlugin!H5P__access_class -> H5P_create_id -> H5T_init
-> H5_init_library -> H5open
Hdf5DataPlugin!FHdf5SimulationReader::OpenFile
ProjectMobius!FProcessSimulationDataRunnable::LoadAndDeserializeHDF5File
- Guard every public reader method with a process-wide recursive
critical section.
- Remove all H5close() calls; the library now lives for the process.
H5Fclose() still closes individual files.
HDF5 simulation files carry the scene geometry in the root wkt_geometry attribute, but selecting one as pedestrian data only loaded the trajectories; the geometry required picking the same file a second time in the Geometry slot (sir306#11). When a selected .h5 contains wkt_geometry, adopt it as the geometry source too, so a single selection loads both. The mesh widget now listens to OnMeshFileChanged so its path display reflects the adopted file. Help texts updated to include .h5 among the supported types.
There was a problem hiding this comment.
Pull request overview
This PR improves the HDF5 (“.h5”) loading workflow so that when an HDF5 agent/trajectory file contains embedded wkt_geometry, selecting it as Pedestrian Vectors also automatically uses it as the Geometry source, eliminating the need to browse the same file twice. It also updates widgets/help text to reflect .h5 support and adds HDF5 reader serialization changes (stacked from #13) to prevent crashes with concurrent access.
Changes:
- Adopt embedded geometry from selected
.h5pedestrian data files by setting the simulation mesh path to the same file whenwkt_geometryis present. - Update
ULoadMeshWidgetto reflect mesh path changes driven externally viaOnMeshFileChanged, and update help/error text to include.h5. - Serialize HDF5 library access in
FHdf5SimulationReaderand avoid process-wideH5close()teardown to prevent SIGSEGV under concurrent readers.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| UnrealFolder/ProjectMobius/Source/MobiusWidgets/Public/UI/LoadSave/LoadMeshWidget.h | Adds NativeDestruct and a mesh-changed handler to keep the widget display in sync with game instance updates. |
| UnrealFolder/ProjectMobius/Source/MobiusWidgets/Private/UI/LoadSave/LoadMeshWidget.cpp | Binds/unbinds OnMeshFileChanged and updates unsupported-type messaging to include .h5. |
| UnrealFolder/ProjectMobius/Source/MobiusWidgets/Private/UI/LoadSave/LoadAgentDataWidget.cpp | Updates unsupported-type messaging to include .h5 for agent data selection. |
| UnrealFolder/ProjectMobius/Source/MobiusCore/Private/Interfaces/ProjectMobiusInterface.cpp | Adds AdoptEmbeddedGeometrySource to reuse HDF5 agent file as geometry source when embedded geometry exists. |
| UnrealFolder/ProjectMobius/Plugins/Hdf5DataPlugin/Source/Hdf5DataPlugin/Private/Hdf5SimulationReader.cpp | Introduces a process-wide lock around HDF5 access and removes H5close() calls to prevent teardown/concurrency crashes. |
Suppressed comments (1)
UnrealFolder/ProjectMobius/Source/MobiusCore/Private/Interfaces/ProjectMobiusInterface.cpp:66
- SetSimulationMeshFilePath() already updates SimulationMeshFileName internally (and broadcasts OnMeshFileChanged). Calling SetSimulationMeshFileName() again here is redundant and risks inconsistencies if the implementations diverge later.
GameInst->SetSimulationMeshFilePath(DataPath);
GameInst->SetSimulationMeshFileName(FPaths::GetCleanFilename(DataPath));
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+57
to
60
| FScopeLock Hdf5Guard(&GetHdf5LibraryLock()); | ||
|
|
||
| // Close any previously open file | ||
| CloseFile(); |
Comment on lines
+55
to
+57
| FString WktGeometry; | ||
| const bool bHasGeometry = Reader.ReadWktGeometry(WktGeometry) && !WktGeometry.IsEmpty(); | ||
| Reader.CloseFile(); |
Collaborator
Author
|
Heads-up: this patches the old |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #11.
Problem
HDF5 simulation files carry the scene geometry in the root
wkt_geometryattribute, but selecting one as Pedestrian Vectors loaded only the trajectories. The geometry required browsing a second time and picking the same file in the Geometry slot. In the reporter's screenshot on #11 the Geometry field still reads "Click Browse to choose file" — agents render, the arena does not.RuntimeMeshBuilder::UpdateMeshFileNamereadsGetSimulationMeshFilePath(), which only an explicit mesh pick sets, and nothing inspected the agent file for embedded geometry.The UI reinforced the confusion:
LoadMeshWidget.cppadvertised.fbx, .obj, .udatasmith, .ifc, .wktwhile the file dialog (NativeFileDialogSubsystem) and the loader (AsyncAssimpMeshLoader.cpp, which routes.h5toProcessMeshFromString) both already accepted.h5.Note this is not platform-specific — the same double selection was required on Windows.
Change
AdoptEmbeddedGeometrySourceinProjectMobiusInterface.cpp: when the selected pedestrian data file is a.h5that containswkt_geometry, set the simulation mesh path to the same file.SetSimulationMeshFilePathalready broadcastsOnMeshFileChanged, whichRuntimeMeshBuilderlistens to, so no further wiring is needed. Files without the attribute are left alone and log a line saying so.ULoadMeshWidgetbindsOnMeshFileChangedso its path display reflects the adopted file instead of remaining on the browse prompt. Unbound inNativeDestruct;AddUniqueDynamicto survive re-construction..h5, matching what the dialog and loader already accept.The probe is cheap:
OpenFileon a Juelich file isH5Fopen+H5Lexists+ a dataspace-dims read — metadata only, no bulk scan.Why the ordering matters
This feature is what makes two threads touch HDF5 at once — the geometry loader (
FAssimpMeshLoaderRunnable) and the trajectory loader (FProcessSimulationDataRunnable) now both open the same file. Without #13 that is a reliable SIGSEGV, because the bundled HDF5 is builtThreadsafety: OFFandCloseFile()used to callH5close(), tearing the library down process-wide. Merging this alone would turn a latent crash into a deterministic one.Verification
macOS, UE 5.5.4, Juelich file with 516 entities / 7.17M trajectory records and a 14-ring polygon (outer boundary, one central hole, 12 obstacles):
One selection, geometry and trajectories both loaded, arena renders with agents moving inside it.
Unrelated issue noticed while testing
The mesh emitter logs
Detected 56 degenerate triangles ... will be droppedfor this file. WKT rings are closed (first point == last), andAsyncAssimpMeshLoader.cppemits 4 wall faces per ring segment including the duplicated closing one — exactly 4 × 14 rings. Harmless today since UE drops them, but the wall emitter assumes open rings. Not addressed here.