search

openframeworks / openFrameworks

openFrameworks is a community-developed cross platform toolkit for creative coding in C++.
http://openframeworks.cc
Other
9.98k stars 2.55k forks source link

ofDirectory sort simplification #8122

Closed dimitre closed 1 month ago

dimitre commented 2 months ago

it uses lambda instead of separate functions, so functionality is more local and it avoids calling twice listDir in sortByDate untested yet. it should perform better as it doesn't duplicate files vector

dimitre commented 2 months ago

Now it is tested, working great. I just don't understand the difference between fast and natural I've added another type that is alphabetical ignoring lower/upper case. called SORT_ALPHABETICAL but we can rename it to SORT_IGNORECASE or something else.

opinions on this one @ofTheo ?

dimitre commented 2 months ago

Merge button is itching on this one but I'll wait a second opinion :)

dimitre commented 1 month ago

shameless ping to @ofTheo ! :)

ofTheo commented 1 month ago

Thanks Dimitre!! I’d definitely recommend comparing results before and after with some larger directories with a mix of file names. Just to make sure there are no regressions.

Also worth doing a timing test, before and after too to make sure this approach is faster or at least not slower :)

ofTheo commented 1 month ago

Just tested vs current ofDirectory::sort

The new method in Debug: dir size 3222 time taken 0.318059 seconds

The old method in Debug: dir size 3222 time taken 0.0430284 seconds

So the old method is 7.3x faster ( at least with the default sorting mode ) on a MBP in Debug

The new method in Release: dir size 3222 time taken 0.288082 seconds

The old method in Release: dir size 3222 time taken 0.0393987 seconds

So the old method is also 7.3x faster in Release.

Also comparing the ordering:

Old on the left and New on the right:

image

We use a ton of png sequences in our projects hence why we are always looking for optimizations with sorting and directory listing.

Also not saying that the old way is more correct but just pointing out it will change behavior with this PR as is.

roymacdonald commented 1 month ago

so the old method is considerably faster. I then don't see the need for this change. The code base is not reduced in complexity by any amount. I don't see this as something that is needed and think we shouldn't merge. Although, @dimitre many thanks for your effort!

ofTheo commented 1 month ago

going to close this for now def would like a faster sort option ( we use dir listing all the time so any improvement helps )