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

Colorimeter correction workflow improvements #328

Merged

Conversation

ethanbrookins
Copy link

Previously, I was unable to create colorimeter corrections through the DisplayCAL GUI, so these changes fix the errors and bugs I encountered in the process.

  1. The i1 Pro 3 is now recognized as a spectrometer and shows as a reference instrument in the dialog.
  2. The most recent spectrometer measurement is correctly saved in the reference file dropdown instead of in the colorimeter dropdown. This means a sequential colorimeter measurement, which automatically triggers the correction creation step, will correctly use the most recent reference measurement.
  3. If using a virtual display like Resolve, that display name will be included in the dialog box. Previously, this would cause an error because that variable was not assigned a value. Note that this is a temporary fix just to solve the problem within the colorimeter correction workflow. This seems to be a byte vs. string problem elsewhere in how the name of the display is added to the ARGYLL_COLPROF_ARGS section of the reference.ti3 file, but I couldn't find the root cause and didn't want to break anything outside of this workflow.
  4. After a .ccmx is created, the colorimeter correction information button now draws the graph in a reasonably sized GUI window. Previously, the window was the full width of the display and could not be resized.
  5. If a .ccmx is loaded that was not created within DisplayCAL (ie. created with the ArgyllCMS ccxxmake command line tool, which is what I was doing before), it can still be graphed. Previously, an error would occur if the file did not contain some additional values that DisplayCAL adds to the file.

Please let me know if any of these changes should be implemented differently or in a more semantically correct way. This is the first I've dabbled into Python and was just trying to solve the immediate error messages I was getting in order to get the workflow usable. Figured I'd start learning by fixing what was bothering me.

@eoyilmaz
Copy link
Owner

O-oh, just released 3.9.12, and I think we should have included these changes in that release. Anyways we can do a quick 3.9.12.1 or 3.9.13 release.

@ethanbrookins
Copy link
Author

Quick update on number 3 in my original comment. I (finally!) found and fixed the root problem of the display name in ARGYLL_COLPROF_ARGS. But now that I have that dialog box back to the default behavior, I'm rethinking slightly how it might allow the user to enter more optional information (or at least how I'd like it to). I'll add a new commit once I fiddle with it a bit more.
So don't merge this just yet.

@eoyilmaz
Copy link
Owner

sure sure, take your time 👍

@ethanbrookins
Copy link
Author

ethanbrookins commented Mar 23, 2024

Alright, here's the new set of improvements.

  1. The ARGYLL_COLPROF_ARGS and ARGYLL_DISPCAL_ARGS sections of .ti3 files are now written with correct formatting and therefore readable to the rest of the app. Previously, the arguments, which are created as byte strings, were being re-encoded at least twice again as byte strings. For example, b'-M "Resolve"' would be changed to b'b\'-M "Resolve"\'', and then to b'b\'b\\\'-M "Resolve"\\\'\'' which meant it was written in plain text to the .ti3 file as b'b\'-M "Resolve"\''. (This took me forever to find haha)
  2. I changed a bit how the colorimeter correction details dialog works. Mainly, it now shows all four options to the user each time and prioritizes information from the reference.ti3. No additional steps are added to the process if the user doesn't want to change anything. But for those that do want to add more data to the correction file, everything is exposed. In particular, I found the previous AutocompleteComboBox for Display Device Manufacturer to be a really awful UX. You could type into it, but couldn't use backspace or the arrow keys to edit. Also, if you typed something that wasn't in the list, presuming that that would allow you to add a custom value, you'd be wrong; nothing was added. Now it's a familiar dropdown list that defaults to Unknown, matching the Display Technology list below it. You can type to jump through the list or just scroll. Also, I updated the pnp.ids to the current list from UEFI and then added six custom manufacturers of production monitors (which are what I'm making LUTs for).
  3. I removed the minimum display update delay for Resolve. The comments mention this was an issue in Resolve 11.x, which is almost a decade old now. This seems to be largely ignored in the actual profile process anyway since Arygll measures the refresh rate each time. I also tested this with the most recent version of Resolve 18.6.6 and the measured refresh rate was essentially identical between dispread and the Resolve GUI.

@eoyilmaz eoyilmaz force-pushed the colorimeter-correction-workflow branch from c02a97a to 602cfc5 Compare March 26, 2024 17:45
DisplayCAL/argyll_instruments.py Outdated Show resolved Hide resolved
DisplayCAL/display_cal.py Outdated Show resolved Hide resolved
DisplayCAL/display_cal.py Outdated Show resolved Hide resolved
DisplayCAL/wxCCXXPlot.py Outdated Show resolved Hide resolved
DisplayCAL/wxCCXXPlot.py Outdated Show resolved Hide resolved
DisplayCAL/wxCCXXPlot.py Outdated Show resolved Hide resolved
DisplayCAL/wxCCXXPlot.py Show resolved Hide resolved
@eoyilmaz eoyilmaz merged commit 842e1ac into eoyilmaz:develop Mar 26, 2024
7 of 8 checks passed
@eoyilmaz
Copy link
Owner

@ethanbrookins thanks for the PR 👍

@ethanbrookins
Copy link
Author

Awesome! Thanks for cleaning this up, @eoyilmaz!

@ethanbrookins ethanbrookins deleted the colorimeter-correction-workflow branch December 15, 2024 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants