Start: create thumbnails using f3d #19489

chennes · 2025-02-08T20:18:48Z

This is based on, and a replacement for, #17069 -- it extends the work in that PR to shift the processing into a set of workers running from the QThreadPool and returning their data as it arrives.

It also adds Start to the C++ test suite, and adds a couple of tests.

chennes · 2025-02-14T00:06:16Z

@andesfreedesign my version of f3d, 2.2.1, does not understand the --config=thumbnail or --verbose=quiet arguments. Which version are you using in your tests?

chennes · 2025-02-14T00:10:51Z

@adrianinsaval could you check this out and make sure it does not freeze up your system when it's generating a lot of previews? I'm just using QThreadPool without doing any extra work, but I can probably do some equivalent of nice to reduce the threads' priority if needed.

src/Mod/Start/App/DisplayedFilesModel.h

src/Mod/Start/App/FileUtilities.h

src/Mod/Start/App/FileUtilities.cpp

adrianinsaval · 2025-02-19T23:09:30Z

Haven't tested big step files yet but this how the thumbnails show up for me on windows, just a logo

chennes · 2025-02-19T23:59:59Z

It didn't even occur to me that this might work on Windows! Definitely didn't test that.

chennes · 2025-02-20T00:03:06Z

@benj5378 what am I supposed to be doing with all these clazy-qstring-allocations warnings?

adrianinsaval · 2025-02-20T02:22:39Z

Haven't tested big step files yet but this how the thumbnails show up for me on windows, just a logo

nevermind, it was just because f3d wasn't on path so wasn't actually being called, once I added the parameter with the binary Path it works but the filename is overlayed on top of the images

I believe that was not the case in the previous PR, maybe because we're not using --config=thumbnail anymore

chennes · 2025-02-20T03:11:58Z

Maybe - I had to remove that because on my Ubuntu 22.04 test system the installed version of f3d didn't support the option. Maybe we should be smarter about it, and detect the version?

benj5378 · 2025-02-20T09:14:22Z

@benj5378 what am I supposed to be doing with all these clazy-qstring-allocations warnings?

As far as I can tell from https://forum.freecad.org/viewtopic.php?t=65560 and https://github.com/KDE/clazy/blob/master/docs/checks/README-qstring-allocations.md , then the idea behind using QLatin1String is QString that have comparative overloads that are faster than QStringLiteral.

However, QString and QLatin1Strnig are different classes, so constructing a QString from QLatin1String means allocating.

Clazy docs:

QString s = QStringLiteral("foo"); // No allocation
QString s = QLatin1String("foo"); // Allocates, use QStringLiteral("foo") instead
s == QLatin1String("foo") // No allocation
s == QStringLiteral("foo") // No allocation

Qt docs:

Note: If the function you're calling with a QLatin1String argument isn't actually overloaded to take QLatin1String, the implicit conversion to QString will trigger a memory allocation, which is usually what you want to avoid by using QLatin1String in the first place. In those cases, using QStringLiteral may be the better option.

So what should you do? You get the warning because you construct a QString from a QLatin1String, thus implicitly calling the constructor and allocating. Change all use of QLatin1String to QStringLiteral, (where we don't have an overload for QLatin1String, nearly all cases).

andesfreedesign · 2025-02-21T13:39:02Z

@andesfreedesign my version of f3d, 2.2.1, does not understand the --config=thumbnail or --verbose=quiet arguments. Which version are you using in your tests?

Sorry, but I was on vacation...
My F3D version is 2.5.0

adrianinsaval · 2025-02-23T15:26:17Z

does --filename=0 option work on your version @chennes ?

adrianinsaval · 2025-02-23T20:05:09Z

--filename=0 --grid=0 --no-background gives a more similar result to --config=thumbnail, we should also set --max-size value, thumbnail config has it at 100Mib on my version, might make sense to make it even lower. For reference here's what I think is used when we use --config=thumbnail option

cat .\thumbnail.d\05_all.json

[
{
  "options":
  {
    "anti-aliasing": true,
    "camera-direction": "-1,-0.5,-1",
    "hdri-ambient": true,
    "max-size":100,
    "no-background": true,
    "verbose": "quiet",
    "tone-mapping": true,
    "translucency-support": true
  }
}
]

cat .\thumbnail.d\10_occt.json

[
{
  "match": ".*(step|stp|iges|igs|brep|xbf)",
  "options":
  {
    "scalar-coloring": true,
    "load-plugins": "occt",
    "up": "+Z",
    "ambient-occlusion": true,
    "coloring-component": "-2",
    "coloring-by-cells": true,
    "camera-direction": "-1,1,-0.5"
  }
}
]

while this is what is used normally:

cat .\config.d\05_all.json

[
{
  "options":
  {
    "axis": true,
    "tone-mapping": true,
    "grid": true,
    "loading-progress": true,
    "anti-aliasing": true,
    "filename": true,
    "camera-direction": "-1,-0.5,-1",
    "hdri-ambient": true,
    "translucency-support": true,
    "animation-progress": true
  }
}
]

cat .\config.d\10_occt.json

[
{
  "match": ".*(step|stp|iges|igs|brep|xbf)",
  "options":
  {
    "scalar-coloring": true,
    "load-plugins": "occt",
    "up": "+Z",
    "ambient-occlusion": true,
    "coloring-component": "-2",
    "coloring-by-cells": true,
    "camera-direction": "-1,1,-0.5"
  }
}
]

we could also consider enabling --camera-orthographic depending on user's view preference

chennes · 2025-02-23T20:27:22Z

Thanks, @adrianinsaval -- can you add that as a GitHub suggestion so I can just adopt the commit?

adrianinsaval · 2025-02-23T20:39:20Z

I just tested a 600mb file, freecad doesn't freeze but I eventually get this warning:
17:28:52 QProcess: Destroyed while process ("C:\\Program Files\\F3D\\bin\\f3d.exe") is still running.
the warning is shown after some time every time I start freecad or more specifically every time Start is loaded
setting max-size will likely greatly reduce how often users can encounter this warning but it might still a good idea to redirect those to log level or straight up ignore them

src/Mod/Start/App/ThumbnailSource.cpp

chennes · 2025-02-24T02:08:12Z

f3d turns out to be something of a moving target: I just tried it on Ubuntu 22.04LTS and it's only version 1.2 there, which doesn't have a "--load-plugins" argument (for example). How much maintenance are we committing ourselves to here?

adrianinsaval · 2025-02-28T02:45:14Z

f3d turns out to be something of a moving target: I just tried it on Ubuntu 22.04LTS and it's only version 1.2 there, which doesn't have a "--load-plugins" argument (for example). How much maintenance are we committing ourselves to here?

what where you testing on before? I don't think we need to support every single feature in 22.04 IMO it's okay to ask for a higher minimum f3d version for this non critical QoL feature. So perhaps it makes sense to have a version check before attempting to run f3d. On the other hand I don't know if the --load-plugins argument is even required, I never needed it when calling f3d 2.5.0 from command line.

chennes · 2025-02-28T13:58:26Z

f3d turns out to be something of a moving target: I just tried it on Ubuntu 22.04LTS and it's only version 1.2 there, which doesn't have a "--load-plugins" argument (for example). How much maintenance are we committing ourselves to here?

what where you testing on before? I don't think we need to support every single feature in 22.04 IMO it's okay to ask for a higher minimum f3d version for this non critical QoL feature. So perhaps it makes sense to have a version check before attempting to run f3d. On the other hand I don't know if the --load-plugins argument is even required, I never needed it when calling f3d 2.5.0 from command line.

I needed the --load-plugins=occt to generate previews for STEP files (which I'd guess is a pretty common use-case).

chennes · 2025-02-28T14:15:31Z

OK: the documentation for pre-2.0 releases is pretty sketchy, and it looks like I can write support for 2.x and 3.x, so I will add a bit of code to do a version check, and cut off at 2.0 as the oldest we support. That was only two years ago, but this software is undergoing quite rapid development.

chennes · 2025-02-28T19:58:56Z

@FreeCAD/code-quality-working-group could you please take a look at ThumbnailSource.*? Particularly to make sure I'm thread-safe there, but anything else that catches your eye as well. Thanks!

chennes · 2025-03-10T16:57:27Z

@pieterhijma is still looking at this.

pieterhijma

The most important question was the synchronization. It looks good but I have some suggestions to make it more clear to other developers.

I also looked at all the other code changes and I made various suggestions. All in all it looks good but I think naming could be improved at some parts (I didn't understand some things at first sight) and I tried to help remove some boilerplate.

src/Mod/Start/App/DisplayedFilesModel.cpp

pieterhijma · 2025-03-12T14:10:59Z

src/Mod/Start/App/ThumbnailSource.cpp

+        Base::Console().Log("Creating thumbnail for %s...\n", _file.toStdString());
+        _process->start(f3d, args);
+        constexpr int checkEveryMs {50};
+        while (!_process->waitForFinished(checkEveryMs)) {


The busy waiting

src/Mod/Start/App/ThumbnailSource.cpp

pieterhijma · 2025-03-12T14:15:01Z

src/Mod/Start/App/ThumbnailSource.cpp

+        "User parameter:BaseApp/Preferences/Mod/Start");
+    const auto f3d = QString::fromUtf8(hGrp->GetASCII("f3d", "f3d").c_str());
+    const QStringList args {QLatin1String("--version")};
+    QProcess process;


The other place where the process is started.

I've added an explanatory comment to the main mutex-protected method to communicate the design decision made here. I believe that blocking is the best thing to do in this initialization method, and no other approach really makes sense.

Yes, I agree, see my comment above about blocking.

pieterhijma · 2025-03-12T14:15:12Z

src/Mod/Start/App/ThumbnailSource.cpp

+    const QStringList args {QLatin1String("--version")};
+    QProcess process;
+    process.start(f3d, args);
+    process.waitForFinished();


blocking here.

I'm blocking here because there's no realistic probability of a user-requested interruption of a call that is only requesting the version output from f3d (it's not asking the software to do a conversion here).

tests/src/Mod/Start/App/ThumbnailSource.cpp

chennes · 2025-03-12T16:55:11Z

Thanks for the review, @pieterhijma -- I'll start making these changes.

chennes · 2025-03-18T04:34:08Z

@pieterhijma I've addressed the majority of your comments above -- exceptions are noted inline. I'm pushing a new commit so you can take a look at the changes, though I'm having some difficulty getting the test code to link on my local build, so I expect it to also fail in CI here.

src/Mod/Start/App/ThumbnailSource.cpp

pieterhijma

Thanks for making the changes! Looks good to me. I just have a couple of small things, see the inline comments.

pieterhijma · 2025-03-24T11:24:41Z

src/Mod/Start/App/DisplayedFilesModel.cpp

-        std::make_pair(int(DisplayedFilesModelRoles::modifiedTime), "modifiedTime"),
-        std::make_pair(int(DisplayedFilesModelRoles::path), "path"),
-        std::make_pair(int(DisplayedFilesModelRoles::size), "size"),
+        std::make_pair(static_cast<int>(DisplayedFilesModelRoles::author), "author"),


Yes ok, I hoped that I could get rid of the make_pair as well, but since that doesn't work, you could argue that my solution adds even more boilerplate.

src/Mod/Start/App/DisplayedFilesModel.cpp

src/Mod/Start/App/FileUtilities.cpp

pieterhijma · 2025-03-24T11:45:15Z

src/Mod/Start/App/ThumbnailSource.cpp

+        QStringList args(_f3dBaseArgs);
+        args << QLatin1String("--output=") + thumbnailPath << _file;
+
+        _process = std::make_unique<QProcess>();


Hm, too bad we need two versions. Actually, I tried to explain that after reading the QProcess documentation, I actually liked the solution with a blocking call, so waitForFinished() because it has this timeout of 30 s. Since we don't need to check for interrupts, we don't need the busy-wait loop with an arbitrary value. I'm also ok with this code, but it has the drawback that if f3d misbehaves and for example enters an endless loop, it will run forever until the application is killed.

pieterhijma · 2025-03-24T11:53:06Z

src/Mod/Start/App/ThumbnailSource.cpp

+        "User parameter:BaseApp/Preferences/Mod/Start");
+    const auto f3d = QString::fromUtf8(hGrp->GetASCII("f3d", "f3d").c_str());
+    const QStringList args {QLatin1String("--version")};
+    QProcess process;


Yes, I agree, see my comment above about blocking.

tests/src/Mod/Start/App/FileUtilities.cpp

chennes · 2025-03-28T02:50:06Z

@pieterhijma your comment about the timeout reminded me that we don't actually properly handle that case, so I've added the code to do so (the timeout doesn't actually kill the process, but the code assumed that after the wait the process had completed, which is not necessarily true). I've also added a timeout to stop the job after 30 seconds so it doesn't just consume this thread forever.

3x380V · 2025-04-01T18:18:59Z

src/Mod/Start/App/FileUtilities.cpp

+
+    // uint64_t can't express numbers higher than the EB range (< 20 EB)
+    constexpr std::array units {"B", "kB", "MB", "GB", "TB", "PB", "EB"};
+    constexpr double unitFactor = 1000.0;


FreeCAD is going to show different file size than Windows File Explorer, Gnome Nautilus, MacOS Finder, etc. Is there particular reason for this behaviour?

More about this imlementation in separate #17817 (comment)

Remember this comment? #17817 (comment)

Not sure how does it answer the above question, so again: why should FreeCAD display different numbers (for sizes up to 1000B numbers would be the same) than each and every file manager ever used?

On your question

Is there particular reason for this behaviour?

The comment answers why it was done like that before

I originally went with the 1000 divisor because I personally prefer the MB to MiB etc. -- the MiB always felt overly pedantic to me (and they are awkward to pronounce :) ). But that is pure personal preference, I'm not going to hold this PR up for that.

I don't think anyone would mind much if you like it to be 1024 and still use MB etc. instead - like in Finder etc.

It is not what I would like, it is common practice. FreeCAD is just showing different size than everything else. I'm not aware about any software using 1000 base. To make it clear, I would of course like it to be denoted according to IEC 80000-13 (kiB, MiB...), just for a fun of educating users, but using kB for 1024, MB for 1024×1024, etc. as semiconductor memory industry does for ages seems appropriate. The key point is to avoid question like why does FreeCAD shows different file size than my file manager.

It looks like I'm wrong, sorry.

Finder does use 1000, not 1024 (since 10.8). The same thing applies for all software on mac which uses the foundation framework for converting number of bytes to a string and doesn't explicitly specify to use 1024: https://developer.apple.com/documentation/foundation/bytecountformatter/countstyle (it always uses MB never MiB)

According to IEC 80000-13, MB and KB is valid to use when using 1000 as factor (look at the examples). It does specify that if 1024 is used, it should be named KiB, MiB etc.

By your arguments (follow standard + consistency with os file explorer), I suppose the current version actually is preferred on macOS?

Ouch! I wasn't aware Apple changed to SI standard in 10.8. Anyway, IEC 80000-13 states in the second paragraph The following referenced documents are indispensable for the application of this document but, duh!, referenced ISO-IEC-2382-16 is no longer valid and I have not purchased current norm.

So indeed, current implementation matches current MacOS.

https://www.iso.org/standard/7259.html has been replaced by https://www.iso.org/standard/63598.html which is available for free ($0 but need to go though the checkout process) or accessed from the online platform: https://www.iso.org/obp/ui/en/#!iso:std:63598:en

chennes requested a review from adrianinsaval February 14, 2025 00:09

chennes mentioned this pull request Feb 14, 2025

Add previews for more file formats in start page using f3d #17069

Closed

chennes marked this pull request as ready for review February 14, 2025 16:58

chennes force-pushed the andesfreedesign-previewsUsingf3d branch from 31dc7c1 to 943cc17 Compare February 14, 2025 21:01

benj5378 reviewed Feb 17, 2025

View reviewed changes

src/Mod/Start/App/DisplayedFilesModel.h Outdated Show resolved Hide resolved

src/Mod/Start/App/FileUtilities.h Outdated Show resolved Hide resolved

src/Mod/Start/App/FileUtilities.cpp Outdated Show resolved Hide resolved

src/Mod/Start/App/FileUtilities.cpp Outdated Show resolved Hide resolved

adrianinsaval reviewed Feb 23, 2025

View reviewed changes

src/Mod/Start/App/ThumbnailSource.cpp Outdated Show resolved Hide resolved

chennes force-pushed the andesfreedesign-previewsUsingf3d branch 5 times, most recently from 753f642 to dccaec5 Compare February 28, 2025 19:57

chennes force-pushed the andesfreedesign-previewsUsingf3d branch from dccaec5 to 197aad7 Compare February 28, 2025 20:04

chennes force-pushed the andesfreedesign-previewsUsingf3d branch 6 times, most recently from 8e2116a to f1672b8 Compare March 1, 2025 02:53

chennes requested a review from pieterhijma March 3, 2025 17:12

pieterhijma suggested changes Mar 12, 2025

View reviewed changes

maxwxyz added move back in the merge queue tag for updating PRs to get them to the end of the merge queue instead of adding more comments. and removed move back in the merge queue tag for updating PRs to get them to the end of the merge queue instead of adding more comments. labels Mar 16, 2025

chennes force-pushed the andesfreedesign-previewsUsingf3d branch from f1672b8 to cfc1737 Compare March 18, 2025 04:33

chennes force-pushed the andesfreedesign-previewsUsingf3d branch 2 times, most recently from b959d82 to 25ffc22 Compare March 18, 2025 12:55

chennes requested a review from pieterhijma March 21, 2025 05:26

adrianinsaval reviewed Mar 22, 2025

View reviewed changes

src/Mod/Start/App/ThumbnailSource.cpp Show resolved Hide resolved

pieterhijma suggested changes Mar 24, 2025

View reviewed changes

andesfreedesign and others added 4 commits March 27, 2025 22:31

Start: add f3d previews

ff576ca

Start: Switch to using QProcess, not std::system

849ae2b

Start: refactor f3d previews, use caching and configurable f3d path

2c9790a

Start: Refactor f3d to use worker threads

34d6363

chennes force-pushed the andesfreedesign-previewsUsingf3d branch from 8e5b568 to 34d6363 Compare March 28, 2025 03:31

chennes marked this pull request as draft March 28, 2025 04:30

3x380V reviewed Apr 1, 2025

View reviewed changes

Start: create thumbnails using f3d #19489

Are you sure you want to change the base?

Start: create thumbnails using f3d #19489

Conversation

chennes commented Feb 8, 2025 • edited Loading

chennes commented Feb 14, 2025

chennes commented Feb 14, 2025

adrianinsaval commented Feb 19, 2025

chennes commented Feb 19, 2025

chennes commented Feb 20, 2025

adrianinsaval commented Feb 20, 2025

chennes commented Feb 20, 2025

benj5378 commented Feb 20, 2025 • edited Loading

andesfreedesign commented Feb 21, 2025

adrianinsaval commented Feb 23, 2025

adrianinsaval commented Feb 23, 2025

chennes commented Feb 23, 2025

adrianinsaval commented Feb 23, 2025

chennes commented Feb 24, 2025

adrianinsaval commented Feb 28, 2025

chennes commented Feb 28, 2025

chennes commented Feb 28, 2025

chennes commented Feb 28, 2025

chennes commented Mar 10, 2025

pieterhijma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chennes commented Mar 12, 2025

chennes commented Mar 18, 2025

pieterhijma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chennes commented Mar 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hyarion Apr 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chennes commented Feb 8, 2025 •

edited

Loading

benj5378 commented Feb 20, 2025 •

edited

Loading

hyarion Apr 2, 2025 •

edited

Loading