Skip to content

Conversation

@smathermather
Copy link
Contributor

No description provided.

@smathermather smathermather marked this pull request as ready for review August 5, 2025 01:45
@NathanMOlson
Copy link
Contributor

NathanMOlson commented Aug 9, 2025

We should change the following to point away from my personal account before merging this PR. Should I open PRs to each of these repos? If so, should I target them to the main branch, or something else?

  • Entwine
  • mvs-texturing
  • OpenSfM
  • PDAL
  • pdal-python (upstream)
  • draco
  • untwine (upstream)

For python dependencies, I've used pip install with --break-system-packages. Is this acceptable? Does anyone know how to do this better?

@smathermather
Copy link
Contributor Author

smathermather commented Aug 9, 2025

We should change the following to point away from my personal account before merging this PR. Should I open PRs to each of these repos? If so, should I target them to the main branch, or something else?

Yes, this pull request isn't ready to merge as-is, but wanted to bundle this as such because it gives us some nominal testing of build by default as well as a place to track changes and discussion.

  • Entwine
  • mvs-texturing
  • OpenSfM
  • PDAL
  • pdal-python
  • draco (currently pointing upstream, which works for my use case but will break some things for others I think)
  • untwine (currently pointing upstream, which works for my use case but will break some things for others I think)

Each of these either needs the ODM specific mods upstreamed where appropriate (e.g. Entwine / Untwine) as Piero indicated or for where that isn't appropriate (e.g. OpenSfM / mvs-texturing), then it needs accompanied by pull requests against OpenDroneMap's forks.

It is also appropriate to keep this scoped to only pull requests against OpenDroneMap's forks, with an eye to upstreaming as a next step.

For python dependencies, I've used pip install with --break-system-packages. Is this acceptable? Does anyone know how to do this better?

Not sure. Since we discourage use outside a container, this might be ok. Although until we have an ODM repo maintainer recruited, that's a change I wouldn't endorse without someone more knowledgeable weighing in on the implications. Were you unable to get it to build without?

@smathermather smathermather marked this pull request as draft August 9, 2025 18:33
@NathanMOlson
Copy link
Contributor

Not sure. Since we discourage use outside a container, this might be ok. Although until we have an ODM repo maintainer recruited, that's a change I wouldn't endorse without someone more knowledgeable weighing in on the implications. Were you unable to get it to build without?

I was unable to get it to build without this flag. This is related to how newer versions of Python prefer to relate to system-installed files. https://stackoverflow.com/questions/75608323/how-do-i-solve-error-externally-managed-environment-every-time-i-use-pip-3

@NathanMOlson
Copy link
Contributor

I just attempted to open a PR to the ODM fork of PDAL, but was rejected with this message:

Pull request creation failed. Validation failed: must be a collaborator.

Any guidance on how I can contribute?

@smathermather
Copy link
Contributor Author

Any guidance on how I can contribute?

I will be offline until Monday, but will check back in then. Just to verify: you did the pull request from your fork of a PDAL repo?

I added classic branch protection rules to the relevant repos (either to main or master) in the meantime. Hopefully this resolves it. Otherwise I'll dive in Monday.

@NathanMOlson
Copy link
Contributor

I will be offline until Monday, but will check back in then. Just to verify: you did the pull request from your fork of a PDAL repo?

I added classic branch protection rules to the relevant repos (either to main or master) in the meantime. Hopefully this resolves it. Otherwise I'll dive in Monday.

Thanks! I originally created a PR from PDAL/PDAL (which does not work), but have now successfully created a PR from NathanMOlson/PDAL.

I'm not sure I understand the branching strategy for all these repos, a pointer to documentation or a brief description of the nominal workflow for making changes would be helpful.

Copy link
Contributor

@NathanMOlson NathanMOlson left a comment

Choose a reason for hiding this comment

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

Re: 9806d91: This pattern is an option when there are only a few minor changes. We use the upstream repository and apply a patch.

This seems like an improvement to me, because I can bump the upstream version, or apply changes to it, without having to send PRs to multiple repos. When the changes are simple, it also seems easier to see exactly what has changed.

I don't think this is a good pattern when the changes are large, like OpenSfM or mvs-texturing.

@smathermather
Copy link
Contributor Author

Re: 9806d91: This pattern is an option when there are only a few minor changes. We use the upstream repository and apply a patch.

I agree this is a good approach when the changes are small.

@NathanMOlson
Copy link
Contributor

@smathermather Do you have a preferred pattern for PRs to the ODM repos that fork outside projects?

My preference would be to have a main branch that tracks the upstream main branch, and an ODM (or develop) branch that tracks main plus whatever modifications ODM requires. Then ODM would pull from these forks using a git tag or commit hash, rather than branch name as seems to be the current pattern.

For this effort, I think it would make most sense to create the ODM branch from upstream main, so we can review the ODM-specific changes rather than all the changes that have happened upstream.

If we go this way, I think the workflow for each repo would be:

  1. Sync main branch with upstream main. (@smathermather or someone else with write privileges)
  2. Create ODM branch from main. (@smathermather or someone else with write privileges)
  3. Port ODM-specific changes, issue pull request to ODM branch (@NathanMOlson)
  4. Review changes and merge (@smathermather or someone else with write privileges)
  5. Update this PR to point to the commit hash of ODM branch after the merge (@NathanMOlson)

This will need to happen for entwine, OpenSfm, mvs-texturing, and draco.

This is just one approach and I'm happy to do something different. @smathermather Let me know if there's a different approach I should take. If we follow my approach, the first two steps in each repo are on you :)

…y v1.26. Now they are built using the venv. fix missing packages. Now making an orthophoto
@Saijin-Naib
Copy link
Contributor

Saijin-Naib commented Oct 6, 2025

@NathanMOlson ,I think next step is to fully remove WebODM and the Python AppData folder from the test machine and re-run.

If it works, the years-old path pollution/escape bug lives on 😤

@NathanMOlson
Good news and bad!

No change when removing WebODM and nuking the AppData\Python folder, so the issue isn't path pollution (yay!)

But, it must be something else. Perhaps OPENCV of the current release doesn't actually have the __version__ attribute as stated in the debug log, and we need to call the version check from cv2 differently?

@Saijin-Naib
Copy link
Contributor

It seems this error might stem from mixed python2/python3 runtimes?
https://forum.opencv.org/t/problem-installing-on-jetson-nano-attributeerror-module-cv2-has-no-attribute-version/3408/2

@NathanMOlson
Copy link
Contributor

But, it must be something else. Perhaps OPENCV of the current release doesn't actually have the __version__ attribute as stated in the debug log, and we need to call the version check from cv2 differently?

In the docker build, cv2.__version__ works as expected:

>>> import cv2
>>> cv2.__version__
'4.12.0'

The same version of OpenCV is built for Windows, so I don't think this is an OpenCV problem.

@smathermather
Copy link
Contributor Author

As windows build and python isolation isn't a new problem, we don't have to get that resolved in this pull request. It's already better from a Windows build perspective, and hopefully what Sam proposes in #1936 can get us 100%.

Thermal testing is done, according to Brett. So now just waiting on some multispectral testing. We have a mapir dataset and some DJI data. Will loop back soon on that.

@smathermather
Copy link
Contributor Author

We've got a good multispectral dataset now, so diving into testing today. 🤞

@smathermather
Copy link
Contributor Author

smathermather commented Oct 20, 2025

Multispectral works. So unless anyone has any objections to merging, speak now, etc.. I'll try to get this into main / build process this week.

Edit: retraction. Multispectral will require troubleshooting. More as I know it.

@smathermather
Copy link
Contributor Author

smathermather commented Oct 20, 2025

@smathermather
Copy link
Contributor Author

smathermather commented Oct 20, 2025

21.04 Multispectral log
24.04 Multispectral log

edit: will redo 21.04 -- looks like that wasn't a full rerun so the logs aren't as useful for comparison.
edit2: these should now each reflect a full rerun.

@smathermather
Copy link
Contributor Author

If I had to guess, this is the warning pointing us in the direction of the issue (line 727 of 24.04 multispectral log):
[WARNING] Cannot compute homography: equalize() got an unexpected keyword argument 'selem'

@sbonaime
Copy link
Contributor

If I had to guess, this is the warning pointing us in the direction of the issue (line 727 of 24.04 multispectral log): [WARNING] Cannot compute homography: equalize() got an unexpected keyword argument 'selem'

in multispectral.py
line 596
im = rank.equalize(im, selem=selem)
=> im = rank.equalize(im, footprint =selem)

https://scikit-image.org/docs/0.23.x/release_notes/release_0.20.html

@smathermather
Copy link
Contributor Author

Cool. That addresses the scikit issue, so band alignment should be fixed. Looks like our output is still distorted, or perhaps just shifted?
distorted_multispectral_patched

Looks like perhaps it's shifted as function of --align not working:

2025-10-21 16:45:56,747 INFO: Saving DSM feature match visualization to: /var/www/data/8339cd43-621c-4f4c-93f0-bed3a5d1b436/opensfm/stats/codem/dsm_feature_matches.png
2025-10-21 16:45:56,794 INFO: Saving DSM feature registration parameters to: /var/www/data/8339cd43-621c-4f4c-93f0-bed3a5d1b436/opensfm/stats/codem/registration.txt
[INFO]    Fine registration...
2025-10-21 16:45:56,795 INFO: Solving ICP registration.
2025-10-21 16:46:07,674 DEBUG: ICP converged via minimum relative change in RMSE.
2025-10-21 16:46:07,687 DEBUG: ICP number of iterations = 17, RMSE = 0.3973072419233715
2025-10-21 16:46:07,687 INFO: Saving ICP registration parameters to: /var/www/data/8339cd43-621c-4f4c-93f0-bed3a5d1b436/opensfm/stats/codem/registration.txt
[WARNING] Cannot compute alignment matrix: 'Filter' object is not subscriptable
[WARNING] Alignment to /var/www/data/8339cd43-621c-4f4c-93f0-bed3a5d1b436/gcp/align.laz will be skipped.
[INFO]    Creating Entwine Point Tile output
[WARNING] Removing previous EPT directory: /var/www/data/8339cd43-621c-4f4c-93f0-bed3a5d1b436/entwine_pointcloud
[INFO]    running entwine build --threads 24 --tmp "/var/www/data/8339cd43-621c-4f4c-93f0-bed3a5d1b436/entwine_pointcloud-tmp" -i /var/www/data/8339cd43-621c-4f4c-93f0-bed3a5d1b436/odm_georeferencing/odm_georeferenced_model.laz -o "/var/www/data/8339cd43-621c-4f4c-93f0-bed3a5d1b436/entwine_pointcloud"
1/1: /var/www/data/8339cd43-621c-4f4c-93f0-bed3a5d1b436/odm_georeferencing/odm_georeferenced_model.laz

Rerunning now with align set to georeferencing to discern if we have two more issues or one.

@NathanMOlson
Copy link
Contributor

The problem is in align.py line 116:

        matrix = np.fromstring(reg['matrix'], dtype=float, sep=' ').reshape((4, 4))

reg is the return value of the codem function ApplyRegistration.get_registration_transformation(), which returned a Dict[str, str] in versions up to 0.25.0. The function returns a pdal.Pipeline since v0.25.1. The current version is 0.26.1.

Still trying to figure out how to get the matrix from the returned pdal.Pipeline.

@NathanMOlson
Copy link
Contributor

The problem is in align.py line 116:

        matrix = np.fromstring(reg['matrix'], dtype=float, sep=' ').reshape((4, 4))

reg is the return value of the codem function ApplyRegistration.get_registration_transformation(), which returned a Dict[str, str] in versions up to 0.25.0. The function returns a pdal.Pipeline since v0.25.1. The current version is 0.26.1.

Still trying to figure out how to get the matrix from the returned pdal.Pipeline.

I just committed an attempt to use the new CODEM API, but I'm not sure this will work. @smathermather if you've got a live environment to test in, you may be able to figure out the magic incantation quicker than me (if this guess is incorrect).

Another option to fix the error is to roll CODEM back to v0.25.0, but I'd rather stick with the new version and figure out the magic incantation. :)

@NathanMOlson
Copy link
Contributor

Actually it looks like get_registration_transformation() returns a pdal.pipeline.Filter, not a pdal.Pipeline like it claims to. I think the most recent commit is likely to work.

@smathermather
Copy link
Contributor Author

Excellent. Building and will test.

@smathermather
Copy link
Contributor Author

We are good to go:
image

@NathanMOlson
Copy link
Contributor

Here's the commit message I would like to apply to this branch before merge:

Changes to support building on Ubuntu 24.04 and windows-2022 Github runner.

Update Python to 3.12 (This requires installing dependencies in virtual environment, and running python scripts from the virtual environment)
Update dependencies (Ubuntu dependencies in snap/snapcraft24.yaml, python dependencies in requirements.txt, windows dependencies built in OpenDroneMap/windows-deps repo)
Update CUDA (to 12.8.1 on Windows, to 13.0.0 on Ubuntu)
Run tests as part of docker build
Use exact commits to specify dependencies that are built from source, instead of branch names
Use upstream versions of PDAL, PDAL-Python, untwine, ExifRead, draco
Build windows builds with -j2
Use Micasense's latest version of dls.py
Update failing unit tests to match current behavior

@smathermather smathermather merged commit 3ea339a into OpenDroneMap:24.04 Oct 22, 2025
2 checks passed
@smathermather
Copy link
Contributor Author

Here's the commit message I would like to apply to this branch before merge:

Done. As we've minimized API changes, I think we can release as 3.6.0, but I would like to hear if there are any protests first.

@smathermather
Copy link
Contributor Author

@NathanMOlson
Copy link
Contributor

@smathermather what's the route to getting this into master?

@smathermather
Copy link
Contributor Author

Good question. We have mild conflicts, but nothing too bad. A simple pull request against master should do it, which is what I should have done from go.

smathermather added a commit that referenced this pull request Oct 28, 2025
* Changes to support building on Ubuntu 24.04 and windows-2022 Github runner.
* Update Python to 3.12
    * Install dependencies in virtual environment
    * Run python scripts from the virtual environment)
* Update dependencies
    * Ubuntu dependencies in snap/snapcraft24.yaml
    * Python dependencies in requirements.txt
    * Windows dependencies built in OpenDroneMap/windows-deps repo
* Update CUDA
    * 12.8.1 on Windows
    * 13.0.0 on Ubuntu)
* Run tests as part of docker build
* Use exact commits to specify dependencies that are built from source, instead of branch names
* Use upstream versions of Libraries:
    * PDAL
    * PDAL-Python
    * untwine
    * ExifRead
    * draco
* Build windows builds with -j2
* Use Micasense's latest version of dls.py
* Update failing unit tests to match current behavior

Co-authored-by: Stephen Mather <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants