-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Add previews for more file formats in start page using f3d #17069
base: main
Are you sure you want to change the base?
Conversation
for more information, see https://pre-commit.ci
.arg(qfi.baseName()); // Temporary path for the thumbnail | ||
|
||
QString command = | ||
QString(QLatin1String("f3d --config=thumbnail --load-plugins=occt --verbose=quiet " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems like it just assumes this is installed and in PATH, do we need it at all? isn't freecad itself capable of generating these thumbnails? Also is this happening in the background? because otherwise it could be quite the slowdown to generate these thumbnails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, FreeCAD only generates its own thumbnails for FCStd files...if you look closely at the Examples files, the Schenkel.step thumbnail is not displayed
For those who work with FreeCAD importing files from other formats, it is very useful to be able to view the thumbnails in "Recent Files"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it currently doesn't do it, my point was that it should be possible to make it although I understand if you are not interested in implementing that. My other points still stand though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if f3d will be used it's path should be configurable because on mac and windows it's not as common to have 3rd party executables in your PATH.
else { | ||
// If it is not a FreeCAD file, generate thumbnail using F3D | ||
QString thumbnailPath = QString(QLatin1String("%1/%2_thumbnail.png")) | ||
.arg(QLatin1String("/tmp")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the path should not be hardcoded, you should be using Application::getTempPath()
I believe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better use https://doc.qt.io/qt-6/qdir.html#tempPath
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we be removing Application::getTempPath()
if it's better to use the one provided by qt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we be removing Application::getTempPath()
No. Application::getTempPath() points to a different directory (can be even customized by the user) than QDir::tempPath() which is not cleaned up by the system after a reboot.
And since the currently created thumbnails of FCStd files are saved to /tmp (under Linux) it makes no sense for me to choose a different directory for f3d thumbnails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a pending PR #18095 that creates the thumbnails in a directory as specified by freedesktop.org: https://specifications.freedesktop.org/thumbnail-spec/0.8.0
Just adding the F3D repology badge to see its spread within the downstream packaging ecosystem: Edit: I've opened a ticket request to update the OpenMandriva F3D package to the latest stable (OpenMandrivaAssociation/distribution#2993) |
@@ -190,16 +190,50 @@ void DisplayedFilesModel::addFile(const QString& filePath) | |||
if (!qfi.isReadable()) { | |||
return; | |||
} | |||
|
|||
// Check if FreeCAD can open the file by its extension |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code below is self-explanatory enough to make this comment obsolete. The same applies to the comments up to line 204
.arg(filePath); // Input file | ||
|
||
// Run the f3d command to generate the thumbnail | ||
int result = std::system(command.toStdString().c_str()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not use system() to run an external process because this will freeze the host application. Use https://doc.qt.io/qt-6/qprocess.html instead.
Just stating the obvious, by accepting this PR we are adding yet another dependency to our long list of dependencies. Also F3D is under the BSD-3 license. Do we need to mention this in the About dialog too ? |
As I understand it f3d would be an optional dependency, but if we will distribute freecad with f3d included we probably should |
see #17121 for an example of calling a binary from PATH by default or from a configured location if available. |
any news on this PR? |
This PR should be re-checked after #17876 has been merged. |
@andesfreedesign ping. Also has some conflicts. |
Hi |
@andesfreedesign next merge meeting is Friday and next Monday. Perhaps you can get the conflicts fixed by then ? |
@andesfreedesign can you address the issues raised by @adrianinsaval and @wwmayer (related to setting explicitly the location of f3d, and to not using std::system). Thanks! |
@andesfreedesign ping for feedback |
@andesfreedesign please address the issues |
I have some fixes here if you want to test it, https://github.com/hasecilu/FreeCAD/tree/feat/f3d_thumbnails |
@andesfreedesign ping for feedback. I'll label this PR as stale so we won't tag you each week. If you come back to this PR just push or comment and we remove the label. |
waiting for review |
For some reason this was not working for me even though I had f3d installed and on PATH, I'll have a closer look tomorrow and try to figure out what is happening |
The purpose of this modification is to be able to display on the Start Page, the thumbnails of 3D files of other formats, in addition to FreeCAD's own (FCStd)
Here you can see the thumbnail of a .stl and .step file that I imported for testing...and even the thumbnail of the Schenkel.stp file, from "Examples" is displayed
For this you must have the F3D viewer installed on the system
https://f3d.app/
"F3D is a fast and minimalist 3D viewer desktop application. It supports many file formats, from digital content to scientific datasets (including glTF, USD, STL, STEP, PLY, OBJ, FBX, Alembic), can show animations and support thumbnails and many rendering and texturing options including real time physically based rendering and raytracing."
Here you can view thumbnails of some of the file formats...
And it is multiplatform (Windows, MacOs, GNU/Linux)