Skip to content
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

Reverting pipe stuff #18360

Merged

Conversation

jenshannoschwalm
Copy link
Collaborator

@wpferguson @TurboGit @kofa73

This would be the proposal based on basic_hash being calculated on position instead of iop_order.

@jenshannoschwalm jenshannoschwalm added bugfix pull request fixing a bug wip pull request in making, tests and feedback needed priority: high core features are broken and not usable at all, software crashes labels Feb 4, 2025
- added/fixed comment about position
- some constify while being here
- a hard-to-read static function name was renamed
As the hash is based upon position in the pipe - this is not iop_order - we have to find and use that.
As dt_dev_pixelpipe_cache_hash() requires the piece position in the pipe - not the iop_order of
the module in question - we have to use that.
- for applied roi_in/out changes
- for applied styles
Only enabled pieces/modules should be integrated into the hash
@TurboGit
Copy link
Member

TurboGit commented Feb 4, 2025

Tested 10 minutes and no issues with the scenario I had reported. Lot better to me. Let's see what @wpferguson has to report with its torture test :)

@wpferguson
Copy link
Member

First test, processed 50 images without any problems. I have a couple more sets of images to process. Back to tortur... err, testing.

@jenshannoschwalm
Copy link
Collaborator Author

Also tested a lot more concentrating on cache correctness (masks and pixelpipe cache hits) and couldn't spot any regression.

@jenshannoschwalm
Copy link
Collaborator Author

Last force-push just fixed clearing darktable.develop->history_last_module twice.

@jenshannoschwalm
Copy link
Collaborator Author

@TurboGit in case this stays all good and the way to go, do you want me to create a 5.0 specific PR?

@jenshannoschwalm jenshannoschwalm removed the wip pull request in making, tests and feedback needed label Feb 5, 2025
@TurboGit
Copy link
Member

TurboGit commented Feb 5, 2025

@TurboGit in case this stays all good and the way to go, do you want me to create a 5.0 specific PR?

Yes please! Are you on the #darktable-dev channel in https://matrix.to/#/#_oftc_#darktable:matrix.org ?

(I'm asking because I've given the plan for 5.0.1 release there - BTW, this is important for all devs to be on this channel now that there is no more dev mailing-list).

@jenshannoschwalm
Copy link
Collaborator Author

No I wasn't until now. Is the dev channel something different from plain darktable?

@TurboGit
Copy link
Member

TurboGit commented Feb 5, 2025

No I wasn't until now. Is the dev channel something different from plain darktable?

Yes, that's another channel that I'm using to prepare the releases.

@victoryforce
Copy link
Collaborator

Small correction, #darktable-dev can be joined at https://matrix.to/#/#darktable-dev:matrix.org

@jenshannoschwalm jenshannoschwalm added this to the 5.2 milestone Feb 5, 2025
@wpferguson
Copy link
Member

Finished testing. Processed 3 sets of images with no problems. By the last set I was just editing and not worrying about problems, so I was going as fast as I normally do. 🎆

It works for me 😄

Thanks

Copy link
Member

@TurboGit TurboGit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @jenshannoschwalm for the very hard work on this issue.

@TurboGit TurboGit merged commit 2b22e83 into darktable-org:master Feb 5, 2025
6 checks passed
@jenshannoschwalm
Copy link
Collaborator Author

It works for me 😄

Thanks

VERY BIG THANKS TO YOU!

btw, after all this time you kept hammering and me trying to isolate the issue a) this now seems a cleaner solution and b) i still could not spot the code part where the translation to iop_order fails :-(

@jenshannoschwalm jenshannoschwalm deleted the reverting_pipe_stuff branch February 5, 2025 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix pull request fixing a bug priority: high core features are broken and not usable at all, software crashes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants