nmoehrle / mvs-texturing

Algorithm to texture 3D reconstructions from multi-view stereo images
Other
931 stars 328 forks source link

Fix technically incorrect comparison in tone mapping. #134

Closed nh2 closed 4 years ago

nh2 commented 4 years ago

Fix for https://github.com/nmoehrle/mvs-texturing/commit/181b33661d813be4daba7802184ee0ce51947daa#r37229123:

Clang 7 warnings show:

libs/tex/generate_texture_patches.cpp:130:9: warning: logical not is only applied to the left hand side of this comparison [-Wlogical-not-parentheses]
    if (!settings.tone_mapping == TONE_MAPPING_NONE) {
        ^                      ~~
libs/tex/generate_texture_patches.cpp:130:9: note: add parentheses after the '!' to evaluate the comparison first
    if (!settings.tone_mapping == TONE_MAPPING_NONE) {
        ^
         (                                         )
libs/tex/generate_texture_patches.cpp:130:9: note: add parentheses around left hand side expression to silence this warning
    if (!settings.tone_mapping == TONE_MAPPING_NONE) {
        ^
        (                     )

! binds tigther than ==, so what was there so far was if ((!settings.tone_mapping) == TONE_MAPPING_NONE) { which is semantically incorrect.

It is still bug-free currently because only values 0 and 1 in enum

enum ToneMapping {
    TONE_MAPPING_NONE = 0,
    TONE_MAPPING_GAMMA = 1
};

but as soon as add a third value will be added, this would be wrong.

This commit fixes it by directly checking TONE_MAPPING_GAMMA for applying the gamma_correct() function.

nmoehrle commented 4 years ago

You are absolutely right, thank you for the detailed explanation :-)