eoyilmaz / displaycal-py3

DisplayCAL Modernization Project
https://eoyilmaz.github.io/displaycal-py3/
GNU General Public License v3.0
854 stars 60 forks source link

Colorimeter correction workflow improvements #328

Closed ethanbrookins closed 7 months ago

ethanbrookins commented 8 months ago

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_COLPROFARGS 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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

sure sure, take your time 👍

ethanbrookins commented 7 months ago

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 commented 7 months ago

@ethanbrookins thanks for the PR 👍

ethanbrookins commented 7 months ago

Awesome! Thanks for cleaning this up, @eoyilmaz!