feat: implement custom STL preview renderer from scratch - #1429
Conversation
CyanVoxel
added
Type: Feature
Computerdores
left a comment
Computerdores
left a comment
There was a problem hiding this comment.
read over the loading code out of curiosity and wrote down some thoughts I had
Note that I didn't think about it particularly long, so take these for the not-thought-through thoughts that they are :)
| def _read_stl_header(filepath: Path) -> bytes: | ||
| with filepath.open("rb") as file: | ||
| return file.read(_BINARY_STL_HEADER_SIZE) |
There was a problem hiding this comment.
this seems unnecessary to me
There was a problem hiding this comment.
It is necessary to keep loading times small by avoiding expensive full file reads. In the common case for a correctly formatted binary STL file, the full file is never read into a python byte object, which is how loading times are kept below 1ms.
There was a problem hiding this comment.
mb on the phrasing, what I meant was having it as a separate function and passing the result around
There was a problem hiding this comment.
Ah I see. Just thought it was clean. It's small but it has its very clear purpose. In general I like small functions bc they're easy to test. We can change it though. Curious, is there any downside to it that I'm not seeing perhaps?
|
@Computerdores thanks for the review! Good points :) Well, do we want to move forward with this approach? There's no point in polishing experimental code until we aim to use it. |
|
Approach-wise this is the furthest anything has come by far, and is actually the first one to get some useful renders (and no crashes so far!). I think this is definitely worth refining as an approach, and I'm shocked that the performance seems decent (it's at least snappy with my 5-50 KB files). |
|
yeah my take is pretty much the same as Cyan's, only thing I am unsure about is the performance Footnotes
|
|
I did three things:
Results for ~100k triangle render (edit: that's a 5MB file if binary format):
Two conclusions:
Thoughts? |
So just to be clear, these C++ experiments referencing bindings aren't present in the most recent commits? Or is this referencing the changes made in 04b11e5? Regardless, I feel it's probably best to have something that works fine first, and then optimize from there in future PRs. On my machine at least, the render time of the STLs I have is extremely reasonable and is comparable to PDFs. I think otherwise the biggest concerns are the minor graphical glitches that occur with some of the faces, which appear to either render in the wrong order, with inverted normals, or otherwise strange stretched triangles. These models all appear normally when viewing in other programs. I have several examples of these: Lastly, I apologize for the merge conflict created with the renaming of the |
|
Correct about the c++ experiments not being present here. iirc I just wrote a standalone cpp script with the same logic, without any intention of actually ever using that code. Never actually wrote any bindings I'm glad you think the version we have now is sufficient. I will look into the merge conflicts. They can't be that bad right (famous last words...) Also I should be able to fix the visual artifacts, and then we should be good to go! May be a bit busy right now since I just started my first real software engineering job and moved countries, but I'm passionate enough about this that I will find the time somehow. |
# Conflicts: # src/tagstudio/previews/stl_renderer.py # src/tagstudio/qt/previews/renderer.py
CyanVoxel
left a comment
CyanVoxel
left a comment
There was a problem hiding this comment.
Approved once the new review comments are addressed.
I'm also going to target this toward the dev branch which contains features meant for 9.7.0, which will give some additional time for me to make any subjective stylistic changes or additional tweaks before this hits main, plus any changes that may result from upstream thumbnail renderer changes (like alpha backgrounds for thumbnails).
Thank you again for your continued work on this!
| @@ -12,6 +13,10 @@ | |||
| from tagstudio.qt.app_settings import AppSettings, Theme | |||
| from tagstudio.qt.cache_manager import CacheManager | |||
|
|
|||
| # QPixmap creation isn't safe to run concurrently across threads; rendering runs on a | |||
| # pool of worker threads, so this serializes just that conversion step. | |||
| _pixmap_conversion_lock = threading.Lock() | |||
|
|
|||
|
|
|||
| class QtFileRenderer(QObject): | |||
| """A Qt-specific wrapper for rendering image previews and thumbnails from files.""" | |||
| @@ -49,9 +54,10 @@ def render( | |||
| is_loading=is_loading, | |||
| is_thumb=is_thumb, | |||
| ) | |||
| qim = ImageQt.ImageQt(image) | |||
| pixmap = QPixmap.fromImage(qim) | |||
| pixmap.setDevicePixelRatio(pixel_ratio) | |||
| with _pixmap_conversion_lock: | |||
| qim = ImageQt.ImageQt(image) | |||
| pixmap = QPixmap.fromImage(qim) | |||
| pixmap.setDevicePixelRatio(pixel_ratio) | |||
There was a problem hiding this comment.
We've never had an issue with this before, and even commenting out these changes I have no problems with the QPixmaps being created on different worker threads on this PR. I tried finding any official Qt documentation on this and found some older documentation from Qt 5, but that restriction no longer appears to be the case for Qt 6, where it instead says "QPainter can be used in a thread to paint onto QImage, QPrinter, QPicture, and (for most platforms) QPixmap".
Is this an actual issue you somehow encountered?
There was a problem hiding this comment.
I actually don't remember if I added it cause I ran into a problem, or just if I thought it'd be best practice. Seems fine without it as you say, so I remove the lock. Thanks for pointing it out!
| records = np.memmap( | ||
| filepath, | ||
| dtype=_BINARY_STL_DTYPE, | ||
| mode="r", | ||
| offset=_BINARY_STL_HEADER_SIZE, | ||
| shape=(triangle_count,), | ||
| ) | ||
| triangles = records["vertices"].astype(np.float32, copy=True) | ||
| del records | ||
| return triangles |
There was a problem hiding this comment.
This could be a single statement if using np.fromfile(), and would just read the records straight into a numpy array without requiring any memory mapping to an intermediate variable or an explicit del operation. Tested working on my machine
| records = np.memmap( | |
| filepath, | |
| dtype=_BINARY_STL_DTYPE, | |
| mode="r", | |
| offset=_BINARY_STL_HEADER_SIZE, | |
| shape=(triangle_count,), | |
| ) | |
| triangles = records["vertices"].astype(np.float32, copy=True) | |
| del records | |
| return triangles | |
| return np.fromfile( | |
| filepath, dtype=_BINARY_STL_DTYPE, count=triangle_count, offset=_BINARY_STL_HEADER_SIZE | |
| )["vertices"] |
| def _parse_bg_color(bg_color: str) -> tuple[int, int, int]: | ||
| """Parses `bg_color` into an RGB triple. | ||
|
|
||
| Raises ValueError rather than STLRenderError: an invalid color is a | ||
| caller argument mistake, not a problem with the STL file being rendered. | ||
| """ | ||
| rgb = ImageColor.getrgb(bg_color) | ||
| if len(rgb) != 3: | ||
| raise ValueError(f"bg_color must resolve to an RGB triple, got {bg_color!r}") | ||
| return rgb |
There was a problem hiding this comment.
The bg_color is hardcoded above in _stl_thumb(), and could just be hardcoded as an RGB tuple there with no need for a conversion method when everything in this file uses the RGB tuple.
(#1e1e1e in RGB is (30, 30, 30) and #FFFFFF is (255, 255, 255), for quick reference)
|
Comments addressed. Thanks for taking the time to code review |
CyanVoxel
left a comment
CyanVoxel
left a comment
There was a problem hiding this comment.
Thank you so much for all your work on this!
3 participants
Summary
This PR experiments with a custom renderer for STL file previews (#351).
STL files only contain triangle information. No shading, material or light, which makes them pretty easy to parse. STL files are pretty rigid and come in two main formats: binary and ASCII. I handle both, then parse the triangles, compute normals (for lighting), project onto 2d, then draw onto a PIL image.
Pros:
Cons:
For now it is still an experiment (hence draft), so I have included some benchmarking code for tracking rendering times. My investigation shows roughly:
small files (0-2000 triangles): <80ms
medium files (2000-100,000): <1000ms
large files (100,000+): several seconds
For now I set a tri count cap of 100,000 and don't render anything above. Eventually the cap should be configurable.
It is also possible to approximate the rendering by reducing the triangle counts (only use a subset of them), but this didn't look good as the objects had visible holes in them.
This is what it looks like (two of the bottom files are not rendered due to the cap):
From my very short and limited testing the renders looks good enough, and the rendering seems performant enough for users to benefit from the feature. Especially with the tri count cap, as well as concurrently rendering many files at a time. Obviously we may need some more rigorous testing to make sure it works well (eg. on low-end systems).
Although I haven't done any research, I can image most STL files being pretty small. Especially if you have many enough to use TagStudio to organize them. If that's true, a slower renderer that by default only renders smaller STL files might work well for now.
Tasks Completed