More refactoring and descriptions #185

soswow · 2025-09-05T03:14:45Z

No description provided.

Signed-off-by: Aleksandr Motsjonov <[email protected]>

soswow · 2025-09-05T03:16:18Z

src/rawtoaces_util/image_converter.cpp

                "raw:pre_mul",
-                OIIO::TypeDesc( OIIO::TypeDesc::FLOAT, 4 ) );
+                OIIO::TypeDesc(
+                    OIIO::TypeDesc::FLOAT, OIIO::TypeDesc::AGGREGATE::VEC4 ) );


Notice use of enums for some of them (OIIO::TypeDesc::AGGREGATE::VEC4)

killed it. 4 is cleaner

src/rawtoaces_util/image_converter.cpp

soswow · 2025-09-05T03:18:57Z

src/rawtoaces_util/image_converter.cpp

        }
        else
        {
+            on_success(); // TODO Verify with Anton.


TODO Verify that lack of on_success in this return true case was a bug or intentional.

Honestly I can't remember the logic there. Should have put a comment. Could you try reproducing visiting that branch? Also, probably checking the size in the branch above where return true; is not necessary anymore, as ArgParse should enforce that.

soswow · 2025-09-05T03:19:29Z

src/rawtoaces_util/image_converter.cpp

+    }
+    return keys;
+}
+


I might be missing something. Not sure if there is an easier way of extracting keys from a Map.

good enough. there is a simpler way in c++20, but we're not there yet

alternatively, you could use 2 separate vectors for your keys and values.

keep in mind that the map stores the data sorted. not a big problem, but you may get the "custom" listed first, not last.

killing it anyway.

soswow · 2025-09-05T03:20:15Z

src/rawtoaces_util/image_converter.cpp

+                  << "'. The following methods are supported: '"
+                  << OIIO::Strutil::join( get_map_keys( wb_methods ), "', '" )
+                  << "'." << std::endl;
        return false;


Two improvements in one. No need for repeating code in if/else + listing available options in error.
Integrating it with actuall help message might be even better.

soswow · 2025-09-05T03:21:11Z

src/rawtoaces_util/image_converter.cpp

+                  << "provided. D55 will be used as default." << std::endl;
+
+        std::string default_illuminant = "D55";
+        settings.illuminant            = default_illuminant;


just replacing a comment with a named variable.

soswow · 2025-09-05T03:22:28Z

src/rawtoaces_util/image_converter.cpp

+    const OIIO::TypeDesc int_vec4_type_desc(
+        OIIO::TypeDesc::INT, OIIO::TypeDesc::AGGREGATE::VEC4 );
+    const OIIO::TypeDesc float_vec2_type_desc(
+        OIIO::TypeDesc::FLOAT, OIIO::TypeDesc::AGGREGATE::VEC2 );


TODO Eithe rmove it up top outside all methods and use them in other methods or use inline in all the places.

or just don't. I'd rather see the type on the line I'm working on, rather than have to search for it. this saves 5 lines of code by making it harder to read.

src/rawtoaces_util/image_converter.cpp

antond-weta · 2025-09-05T03:26:11Z

include/rawtoaces/image_converter.h

 bool collect_image_files(
    const std::string &path, std::vector<std::vector<std::string>> &batches );

+struct CameraIdentifier


Nice! Can this live in the .cpp?

antond-weta · 2025-09-05T03:28:31Z

include/rawtoaces/image_converter.h

+    std::string model;
+
+    CameraIdentifier() = default;
+    CameraIdentifier( const std::string &_make, const std::string &_model )


this constructor is not strictly needed, you can initialise the struct members directly
camera_identifier = { "ARRI", "too_expensive_for_me" };

antond-weta · 2025-09-05T03:36:41Z

src/rawtoaces_util/image_converter.cpp

+        auto matrix2_attr = image_spec.find_attribute(
+            key2,
+            OIIO::TypeDesc(
+                OIIO::TypeDesc::FLOAT, OIIO::TypeDesc::AGGREGATE::MATRIX44 ) );


have you confirmed that this works? that float[4][4] has the correct order of items when read from float[16]? A zero or identity matrix is not a sufficient test

I Am killing this anyway, but I thought this is just a fency variable that holds value 16

enum AGGREGATE { SCALAR = 1, ///< A single scalar value (such as a raw `int` or ///< `float` in C). This is the default. VEC2 = 2, ///< 2 values representing a 2D vector. VEC3 = 3, ///< 3 values representing a 3D vector. VEC4 = 4, ///< 4 values representing a 4D vector. MATRIX33 = 9, ///< 9 values representing a 3x3 matrix. MATRIX44 = 16 ///< 16 values representing a 4x4 matrix. };

antond-weta · 2025-09-05T03:47:41Z

src/rawtoaces_util/image_converter.cpp

        }
        else
        {
+            on_success(); // TODO Verify with Anton.


Honestly I can't remember the logic there. Should have put a comment. Could you try reproducing visiting that branch? Also, probably checking the size in the branch above where return true; is not necessary anymore, as ArgParse should enforce that.

antond-weta · 2025-09-05T03:55:11Z

src/rawtoaces_util/image_converter.cpp

+    }
+    return keys;
+}
+


good enough. there is a simpler way in c++20, but we're not there yet

alternatively, you could use 2 separate vectors for your keys and values.

keep in mind that the map stores the data sorted. not a big problem, but you may get the "custom" listed first, not last.

antond-weta · 2025-09-05T03:55:47Z

src/rawtoaces_util/image_converter.cpp

-        settings.WB_method = Settings::WBMethod::Custom;
-    }
-    else
+    static std::map<std::string, Settings::WBMethod> wb_methods = {


you may want it to const as well

antond-weta · 2025-09-05T04:00:09Z

src/rawtoaces_util/image_converter.cpp

+    const OIIO::TypeDesc int_vec4_type_desc(
+        OIIO::TypeDesc::INT, OIIO::TypeDesc::AGGREGATE::VEC4 );
+    const OIIO::TypeDesc float_vec2_type_desc(
+        OIIO::TypeDesc::FLOAT, OIIO::TypeDesc::AGGREGATE::VEC2 );


or just don't. I'd rather see the type on the line I'm working on, rather than have to search for it. this saves 5 lines of code by making it harder to read.

Signed-off-by: Aleksandr Motsjonov <[email protected]>

src/rawtoaces_util/image_converter.cpp

antond-weta · 2025-09-05T22:27:53Z

src/rawtoaces_util/image_converter.cpp

-                                      camera_make + "', model = '" +
-                                      camera_model + "'";
+                                      camera_identifier.make + "', model = '" +
+                                      camera_identifier.model + "'";


I suspect we'll end up having to print out make='Sony', model='who_cares' in more than just this place, especially in higher verbosity modes. You may want to move this to CameraIdentifier.description(), or CameraIdentifier.to_string()

antond-weta · 2025-09-05T22:31:00Z

src/rawtoaces_util/image_converter.cpp

-    auto demosaic_algorithm = arg_parser["demosaic"].get();
-    static std::set<std::string> demosaic_algorithms = {
+    auto                         demosaic       = arg_parser["demosaic"].get();
+    static std::set<std::string> demosaic_algos = {


nah, put that back

yeah. that was accedental.

antond-weta · 2025-09-05T22:31:47Z

src/rawtoaces_util/image_converter.cpp

-                  << ". The following methods are supported: "
+                  << "ERROR: unsupported demosaicing algorithm '" << demosaic
+                  << "'. "
+                  << "The following algorithms are supported: "


here is a good spot to use join()?

antond-weta · 2025-09-05T22:47:29Z

src/rawtoaces_util/image_converter.cpp

+    return std::find( vec.begin(), vec.end(), value ) != vec.end();
+}
+
+static std::vector<std::string> supported_wb_methods = {


You could have put these static methods inside the Settings, close to there the enums are defined.

Or try something like this https://stackoverflow.com/a/31836401

Signed-off-by: Aleksandr Motsjonov <[email protected]>

…inants Signed-off-by: Aleksandr Motsjonov <[email protected]>

soswow · 2025-09-06T00:30:33Z

src/rawtoaces_util/image_converter.cpp

-        std::cerr << std::endl;
+        std::cout << std::endl
+                  << "The following illuminants are supported:" << std::endl
+                  << OIIO::Strutil::join( illuminants, "\n" ) << std::endl;


found this other place wher join can be used

soswow · 2025-09-06T00:31:42Z

src/rawtoaces_util/image_converter.cpp

+        std::cout << std::endl
+                  << "The following illuminants are supported:" << std::endl
+                  << OIIO::Strutil::join( illuminants, "\n" ) << std::endl;
+        exit( 0 );


fixes the problem of showing help message for these two commands. Returning false would cause for programm to exit with 1 in main, which semantically incorrect.

soswow · 2025-09-06T00:32:49Z

src/rawtoaces_util/image_converter.cpp

-                                      camera_make + "', model = '" +
-                                      camera_model + "'";
+        const std::string data_type =
+            "spectral data for camera " + camera_identifier;


As suggested, I've implemented toString operator in CameraIdentifier

soswow · 2025-09-06T00:35:51Z

src/rawtoaces_util/image_converter.cpp

+    operator+( const std::string &lhs, const CameraIdentifier &rhs )
+    {
+        return lhs + static_cast<std::string>( rhs );
+    }


Can't claim I know how it's done in c++. That's what I came up with so I can do "mystring" + this_object. I believe it would need smth like

// Stream insertion operator friend std::ostream& operator<<(std::ostream& os, const CameraIdentifier& camera) { return os << camera.make << " " << camera.model; }

if we want to use it in "some string" << this_object scenario. But I am not adding it now

antond-weta

looks good

* More refactoring and descriptions Signed-off-by: Aleksandr Motsjonov <[email protected]> * PR review comments address Signed-off-by: Aleksandr Motsjonov <[email protected]> * changes to be deleted in next commit Signed-off-by: Aleksandr Motsjonov <[email protected]> * revert to good old copy paste and simpler code Signed-off-by: Aleksandr Motsjonov <[email protected]> * More PR comment address Signed-off-by: Aleksandr Motsjonov <[email protected]> * Fix bug where help is printed out for --list-cameras and --list-illuminants Signed-off-by: Aleksandr Motsjonov <[email protected]> --------- Signed-off-by: Aleksandr Motsjonov <[email protected]> Signed-off-by: Savitha M <[email protected]>

More refactoring and descriptions

dbc1b70

Signed-off-by: Aleksandr Motsjonov <[email protected]>

soswow commented Sep 5, 2025

View reviewed changes

src/rawtoaces_util/image_converter.cpp Show resolved Hide resolved

soswow commented Sep 5, 2025

View reviewed changes

src/rawtoaces_util/image_converter.cpp Show resolved Hide resolved

soswow commented Sep 5, 2025

View reviewed changes

src/rawtoaces_util/image_converter.cpp Show resolved Hide resolved

antond-weta reviewed Sep 5, 2025

View reviewed changes

soswow added 3 commits September 6, 2025 00:38

PR review comments address

bff0233

Signed-off-by: Aleksandr Motsjonov <[email protected]>

changes to be deleted in next commit

02e80c5

Signed-off-by: Aleksandr Motsjonov <[email protected]>

revert to good old copy paste and simpler code

ccd7214

Signed-off-by: Aleksandr Motsjonov <[email protected]>

soswow commented Sep 5, 2025

View reviewed changes

src/rawtoaces_util/image_converter.cpp Show resolved Hide resolved

antond-weta reviewed Sep 5, 2025

View reviewed changes

soswow added 2 commits September 6, 2025 09:44

More PR comment address

2c76502

Signed-off-by: Aleksandr Motsjonov <[email protected]>

Fix bug where help is printed out for --list-cameras and --list-illum…

6b6ca55

…inants Signed-off-by: Aleksandr Motsjonov <[email protected]>

soswow commented Sep 6, 2025

View reviewed changes

antond-weta approved these changes Sep 6, 2025

View reviewed changes

antond-weta merged commit 5bf5db5 into AcademySoftwareFoundation:main Sep 6, 2025
13 checks passed

More refactoring and descriptions #185

More refactoring and descriptions #185

Uh oh!

Conversation

soswow commented Sep 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soswow Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soswow Sep 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soswow Sep 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antond-weta left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

soswow Sep 5, 2025 •

edited

Loading

soswow Sep 6, 2025 •

edited

Loading

soswow Sep 6, 2025 •

edited

Loading