Skip to content

Overload pybind11 type_caster from int to C++ uint types to support -1 for pycolmap id types #3071

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
B1ueber2y wants to merge 4 commits into from
Closed

Overload pybind11 type_caster from int to C++ uint types to support -1 for pycolmap id types #3071

B1ueber2y wants to merge 4 commits into from

Conversation

@B1ueber2y
Copy link
Contributor

@B1ueber2y B1ueber2y commented Dec 22, 2024 •
edited
Loading

No description provided.

@B1ueber2y
Copy link
Contributor Author

B1ueber2y commented Dec 22, 2024 •
edited
Loading

This update only supports -1 and positive values and add explicit logs. Example:

In [1]: import pycolmap

In [2]: p = pycolmap.Point2D()

In [3]: p.point3D_id = -2
W20241222 14:41:54.504066 1408914 pybind11_extension.h:59] Error! Only positive values and -1 (as invalid) are supported.
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
Cell In[3], line 1
----> 1 p.point3D_id = -2

TypeError: (): incompatible function arguments. The following argument types are supported:
    1. (self: pycolmap._core.Point2D, arg0: uint64_t) -> None

Invoked with: Point2D(xy=[0, 0], point3D_id=-1), -2

In [4]: p.point3D_id = -1

In [5]: p.point3D_id
Out[5]: 18446744073709551615

In [6]:

@B1ueber2y B1ueber2y changed the title Overload pybind11 type_caster from int to C++ uint types to support -1 as input in pycolmap Overload pybind11 type_caster from int to C++ uint types to support -1 for pycolmap id types Dec 22, 2024
@B1ueber2y
Copy link
Contributor Author

B1ueber2y commented Dec 22, 2024 •
edited
Loading

The stub seems to break after this update since it cannot parse uint32_t and uint64_t: https://github.com/colmap/colmap/actions/runs/12454820180/job/34766524324?pr=3071. No idea on this since I never used this stub feature. Help needed : )

ahojnnes
ahojnnes approved these changes Dec 22, 2024
View reviewed changes
Copy link
Contributor

@ahojnnes ahojnnes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks very much.

sarlinpe
sarlinpe requested changes Dec 22, 2024
View reviewed changes
src/pycolmap/pybind11_extension.h
return false; \
} \
int64_t val = py::cast<int64_t>(src); \
if (val == -1) { \
Copy link
Member

@sarlinpe sarlinpe Dec 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach accepts -1 as valid input for any function that expects uint*_t, while this is valid only for ID setters. Instead, how about wrapping Set{Image,Camera,...}Id with a templated function that accepts an int and casts appropriately?

Copy link
Contributor Author

@B1ueber2y B1ueber2y Dec 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of pycolmap ids actually happen in a lot of places in the bindings (not only for the setters). For example, one can do grep -r '_id' ./src/pycolmap/ and find a lot of relevant bindings. And the logic is also not consistent (e.g., here there is explicit casting supported for only one method: https://github.com/colmap/colmap/blob/main/src/pycolmap/sfm/incremental_mapper.cc#L358).

Actually I have a different proposal: how about we not supporting -1 as input and always recommend users to use ctypes to convert from the Python side themselves, as the solution I suggested in the issue:

import pycolmap
import ctypes
p = pycolmap.Point2D()
p.point3D_id = ctypes.c_uint64(-1).value

@B1ueber2y B1ueber2y marked this pull request as draft December 23, 2024 00:28
@B1ueber2y
Copy link
Contributor Author

B1ueber2y commented Dec 23, 2024

The fix in this PR actually limits the range of uint64_t from 2^64 - 1 to 2^63 - 1 due to the int64_t in the middle. This will also be the case for explicit casting with int as input. So I suggest we not go ahead with the update and stick with p.point3D_id = ctypes.c_uint64(-1).value. We might want to mention it somewhere in the documentation.

@ahojnnes
Copy link
Contributor

ahojnnes commented Dec 23, 2024

How about an explicit INVALID_POINT3D_ID Python/C++ type?

@sarlinpe
Copy link
Member

sarlinpe commented Dec 23, 2024

Using ctypes requires knowing the C++ type of the attribute (utin32 vs uint64), which is not reflected in the Python doc (int only) and cannot be asked users to know. Binding the invalid values seems like a better option, since we can then provide a nicer representation (e.g. pycolmap.invalid_image_id) to some default values, e.g. in Image.__init__ (which for now is displayed as 4294967295, see doc)

colmap/src/colmap/util/types.h

Lines 98 to 103 in cd97b65

const camera_t kInvalidCameraId = std::numeric_limits<camera_t>::max();
const image_t kInvalidImageId = std::numeric_limits<image_t>::max();
const image_pair_t kInvalidImagePairId =
std::numeric_limits<image_pair_t>::max();
const point2D_t kInvalidPoint2DIdx = std::numeric_limits<point2D_t>::max();
const point3D_t kInvalidPoint3DId = std::numeric_limits<point3D_t>::max();

colmap/src/pycolmap/scene/image.cc

Lines 46 to 51 in cd97b65

.def(py::init(&MakeImage<Point2D>),
"name"_a = "",
py::arg_v("points2D", Point2DVector(), "ListPoint2D()"),
"cam_from_world"_a = py::none(),
"camera_id"_a = kInvalidCameraId,
"id"_a = kInvalidImageId)

@B1ueber2y B1ueber2y mentioned this pull request Dec 23, 2024
Merged
@B1ueber2y B1ueber2y closed this Dec 23, 2024
@B1ueber2y B1ueber2y deleted the fix/pycolmap_uint branch December 23, 2024 14:54
B1ueber2y added a commit that referenced this pull request Dec 23, 2024
@B1ueber2y
Add binding support for invalid values in pycolmap id types (#3072)
e924f0f 
Fixes: #3070

Per discussion from #3071
HernandoR pushed a commit to HernandoR/colmap that referenced this pull request Dec 30, 2024
@B1ueber2y
Add binding support for invalid values in pycolmap id types (colmap#3072
7821929 
)

Fixes: colmap#3070

Per discussion from colmap#3071
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sarlinpe sarlinpe sarlinpe requested changes

@ahojnnes ahojnnes ahojnnes approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@B1ueber2y @ahojnnes @sarlinpe