Every worker copies the whole ifc file into memory#1
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves IFC-to-mesh conversion robustness and UX by switching IFC geometry extraction to IfcOpenShell’s geometry iterator (avoiding per-worker full IFC loads), adding GUI input validation/selection guards, and improving overwrite/error handling in the GUI flow.
Changes:
- Reworked IFC mesh extraction to use
ifcopenshell.geom.iterator, added progress/memory reporting, and chunked mesh aggregation (pointcloud_tools/ifc.py). - Improved GUI validation and dialogs for IFC conversion (worker count parsing, required element-type selection, clearer labels, and explicit error on no mesh) (
pointcloud_tools/gui.py). - Added
psutildependency and a dev-only Compose volume mount (requirements.txt,docker-compose.yml).
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| requirements.txt | Adds psutil dependency used for runtime memory reporting in IFC conversion. |
| pointcloud_tools/ifc.py | Replaces multiprocessing batch extraction with IfcOpenShell iterator-based extraction; adds chunking and memory/progress handling. |
| pointcloud_tools/gui.py | Adds safer GUI handling for IFC workers input, element-type selection validation, clearer labeling, and conversion failure reporting. |
| docker-compose.yml | Adds a local host volume mount for map data inside the container. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - ./:/app | ||
| - /tmp/.X11-unix:/tmp/.X11-unix:ro | ||
| - ${XAUTHORITY:-$HOME/.Xauthority}:/tmp/.Xauthority:ro | ||
| - ~/maps:/app/data:rw |
There was a problem hiding this comment.
~/maps in a Compose volume typically isn’t expanded by Docker Compose (it’s not a shell), so this mount may fail or create a literal ~ directory depending on environment. Prefer ${HOME}/maps:/app/data:rw (or a relative path) to make the mapping reliable across setups.
| - ~/maps:/app/data:rw | |
| - ${HOME}/maps:/app/data:rw |
| vars = {} | ||
| for i, t in enumerate(sorted(types.keys())): | ||
| var = tk.BooleanVar(value=True) | ||
| tk.Checkbutton(sel, text=t, variable=var).grid(row=i, column=0, sticky="w") | ||
| tk.Checkbutton(sel, text=f"{t} ({types[t]})", variable=var).grid( | ||
| row=i, column=0, sticky="w" | ||
| ) | ||
| vars[t] = var |
There was a problem hiding this comment.
Avoid naming this dict vars because it shadows Python’s built-in vars() function, which can make debugging harder in this scope. Rename to something like type_vars or checkbox_vars.
| vars = {} | ||
| for i, t in enumerate(sorted(types.keys())): | ||
| var = tk.BooleanVar(value=True) | ||
| tk.Checkbutton(sel, text=t, variable=var).grid(row=i, column=0, sticky="w") | ||
| tk.Checkbutton(sel, text=f"{t} ({types[t]})", variable=var).grid( | ||
| row=i, column=0, sticky="w" | ||
| ) | ||
| vars[t] = var |
There was a problem hiding this comment.
Avoid naming this dict vars because it shadows Python’s built-in vars() function, which can make debugging harder in this scope. Rename to something like type_vars or checkbox_vars.
| if available_memory_gb < MEMORY_ABORT_THRESHOLD_GB: | ||
| print("\nWARNING: Low memory condition detected!") | ||
| print( | ||
| f" Only {available_memory_gb:.1f} GB available " | ||
| f"(need ~{MEMORY_ABORT_THRESHOLD_GB} GB)" | ||
| ) | ||
| print(" Proceeding without visualization to minimize memory usage") | ||
| show_result = False | ||
| elif available_memory_gb < MEMORY_WARNING_THRESHOLD_GB * 2: | ||
| print("\nWARNING: Memory is constrained. Mesh visualization may be slow.") |
There was a problem hiding this comment.
The memory thresholds are inconsistent: MEMORY_ABORT_THRESHOLD_GB (4.0) is larger than MEMORY_WARNING_THRESHOLD_GB (1.0), which makes the elif available_memory_gb < MEMORY_WARNING_THRESHOLD_GB * 2 branch unreachable (the < MEMORY_ABORT_THRESHOLD_GB branch will always trigger first). Also note the same abort threshold is used earlier during iteration to raise MemoryError, so this effectively aborts conversion on machines with <4GB free even though this block implies conversion can proceed without visualization. Consider separating “abort conversion” vs “disable visualization” thresholds and ordering the conditions so the warning path can run.
This pull request improves the robustness and user experience of the IFC-to-mesh conversion workflow in the GUI, adds better error handling, and enhances the clarity of user interface elements. The main changes focus on validating user input, providing clearer prompts, and ensuring that the conversion process handles errors gracefully.
User input validation and error handling:
pointcloud_tools/gui.py).pointcloud_tools/gui.py). [1] [2]pointcloud_tools/gui.py). [1] [2]User interface improvements:
pointcloud_tools/gui.py).pointcloud_tools/gui.py). [1] [2]Development environment:
~/mapsdirectory as a read-write volume to/app/datain the Docker Compose configuration, facilitating access to local map data during development (docker-compose.yml).