Pinhole camera with distortion

[kidal5]

Hello @asmaloney,

I have implemented the functionality to both read and write pinhole camera distortion parameters in accordance with the standard extension. You can refer to the specification here: E57_LEICA_Camera_Distortion.txt.

Additionally, I have added tests to verify the correctness of my implementation. I have also tested reading other files, although I cannot share them publicly.

I do have one question regarding your library's design choices. To handle the optional nature of the distortion parameters, I introduced an initialized field within the PinholeCameraDistortion struct to indicate whether the distortion parameters have been set. Would you consider this the correct approach? I would have preferred to use std::optional<PinholeCameraDistortion>, but this requires C++17 support.

Also, what is the workflow for adding test data into test data repository.

I hope you find this pull request useful and look forward to its review and possible merge.

[kidal5]

Hi @asmaloney ,
Just wanted to follow up on this PR — do you have any plans to review it soon, or could you share the current status? Let me know if there's anything you'd like me to adjust or clarify further.

Thanks again!

[asmaloney]

Hi Vladislav! Thank you for your PR. This library is not a priority for me, so I may not get to a full review for a while. (And I have been away/mostly offline for a couple of months.)

To handle the optional nature of the distortion parameters, I introduced an initialized field ...

I think I would prefer a pointer to a struct that's allocated if the extension is active. I have ideas on how extensions should be handled in the "simple api" case. The current method of just piling everything into the main structs is not a great plan. A pointer would make changes easier for people later on if I get around to handling this properly.

Also, what is the workflow for adding test data into test data repository.

You can do a pull request in the other repo or we can arrange for you to send me the file(s) and I can add them. The key problem with the test repo is going to be the size, so I would prefer the smallest examples you have that are useful for testing.

[asmaloney]

Hey Vladislav.

I mentioned a couple of things in my last response: (1) switching this code to using a pointer to a struct that's allocated if the extension is active and (2) submitting test files for the test repo.

If you have the time to do that, I can take a look at reviewing & merging this in the next release (3.4).

[kidal5]

Hello @asmaloney,

I’ve updated the code, and it now uses a pointer to the struct instead of the initialized member field. I used a unique_ptr for this purpose—does that look okay? Otherwise, I would need to add a destructor to the Image2D struct, which felt unnecessary.

Regarding the second point: the writer test in the ./testE57 binary generates the test files needed. So it would probably be best if you created the PR to the test-data repo.

About the generated test file: its size is 122K. Is that acceptable? If not, the main storage cost comes from the attached image file, which isn’t needed for this test. Currently, I reused the existing images/image.jpg file, but I could replace it with a 1×1px dummy image, which would work fine for this test case.

Lastly, in the official extension specification code examples, they always first check if a field is defined before reading it:

if(distortion.isDefined("dist:CV_P1"))
    CV_P1 = FloatNode(distortion.get("dist:CV_P1")).value();

What do you think—should I follow this pattern, or is my current implementation sufficient?

[asmaloney]

Thanks Vladislav! I'll make some time to review this (and answer your questions) over the next couple of weeks.

[asmaloney]

@kidal5 Could you please rebase this onto master?

[kidal5]

Sure, just rebased.

[asmaloney]

The changes I'm requesting are mostly naming. I want to ensure that it's clear when data is from an extension. (The normals stuff should have been handled like this as well, alas that ship has sailed for now...)

If you have a 1x1 PNG or JPEG handy, we should add that to the test data repo and use it for creating these test files.

[kidal5]

All requested changes are added. Also created pull request into test-data repository with test files.

[kidal5]

Updated the code - now all CI checks should finish.

[asmaloney]

Thanks Vladislav! 🎆