Page MenuHome

Fix T82460: Color Management Curves do not update when Image/UV Editor is present
ClosedPublic

Authored by Philipp Oeser (lichtwerk) on Nov 13 2020, 10:11 PM.

Details

Summary

Caused by rB4212b6528afb.

'updateGLSLCurveMapping()' compares cacheIDs and in certain scenarios,
these are the same when they should not.

  • whenever we had multiple viewports that are colormanaged with curvemappings this worked right (cacheIDs were different)
  • for example, this also worked right when the ImageEditor displays a Render Result or a Compositor Viewer
  • but it worked wrong when the Image Editor displays any other Image (or no Image at all)
  • it also worked right if there were multiple Image Editors [and one of them displays a Render Result e.g]

Now why is this so?

For comparison, the curve mapping's pointer/address is used.

  • update_glsl_display_processor frees the curve_mapping, see BKE_curvemapping_free(global_glsl_state.curve_mapping)
  • similar, update_glsl_display_processor creates a new curvemapping, see BKE_curvemapping_copy(view_settings->curve_mapping)
  • now for the situation that a viewport with curvemapping and a viewport without curvemapping is present and you make changes to the curvemapping the following happens:
    • curve_mapping_settings->cache_id is set once [to the memory address of curvemapping before change]
    • change happens
    • viewport 1 frees curvemapping
    • viewport 2 duplicates using BKE_curvemapping_copy, but this one gets the same address like before the change :/
    • this means we have different data on the same address with the same cacheID...

Solution: to really make the cache ID unique we can combine the pointer with its 'changed_timestamp' [which increases on every change]. Other possibility would be to use a hash but the former sounds safe enough?

Diff Detail

Repository
rB Blender

Event Timeline

Philipp Oeser (lichtwerk) requested review of this revision.Nov 13 2020, 10:11 PM

Overall direction lgtm we might want to add a function to update the cache is to hide the implementation and use a different type that is more suitable for hashes.

source/blender/imbuf/intern/colormanagement.c
4047

Size_t seems incorrect now. Perhaps create a function to calc the cached and use a different type.

Patch seems to be based on master. Best to apply it to blender-v2.91-release and merge it back.

source/blender/imbuf/intern/colormanagement.c
4047

Not important as it is only used here.

This revision is now accepted and ready to land.Nov 16 2020, 11:53 AM