Disable scale solving during alignment#2054
Conversation
The reconstruction and the alignment reference are both expressed in the same metric CRS by the time codem compares them, so the scale between them is 1 by construction. Leaving the scale free lets RANSAC fit degenerate solutions on feature-poor scenes (alignment fails) or absorb noise into a nonphysical scale factor (outputs are silently rescaled by 10-20%). Solving only for rotation and translation makes registration robust on scenes where it currently fails and guarantees alignment preserves the reconstruction's metric accuracy.
|
Here are the three orthophotos generated from the brighton oats runs: Master: master_orthophoto.tif It's clear when loading these in QGIS that the alternative fix of reverting to |
|
This is a great analysis. Pull request to update the dataset: If I understand the problem correctly, this may mean that older tests with align will be more likely to fail, but newer ones (e.g. with dspsift) will be more likely to succeed. edit: |
|
Thanks!
Yes, but mostly because of a quirk of the test. As we add more features or change algorithms the DSM will change. Eventually those changes build up to be large enough that the feature matching fails to find a good fit. But this is change relative to something, and in the test case that's files generated around 2022 (~v3.0.2). In some ways this is exactly the value of the test. Feature matching against two identical images would be easy - the drift between versions has made the test harder over time but that doesn't make it broken. This PR allows us to work with a broader range of inputs.
So actually, given the above, I would argue to leave the dataset in its current state. As it is, the test is effectively performing the use case of "I generated this map with ODM 4 years ago, and I want to now see the difference". That is, it makes sense for The other doubt for me is that with an updated dataset, I suspect the test would pass without this PR (same-version references nearly always match). That means the test would stop acting as a regression test for this bug.
As far as I can see we only create a |
|
Makes sense, although I think the purpose of the test is whether align is working generally, and secondarily, it helped us find another issue. So, the oats test probably needs rewritten to generate the dsm and pc, and then run again to align to those outputs, rather than using a static dataset. Or better yet, use a different, slightly larger dataset that can be split into two subsets to be aligned with CODEM. Edit: without looking (away from keyboard), this should be the only use of CODEM. |
2 participants
The OATS brighton alignment tests (
--alignwith a GeoTIFF and a LAZ reference) have been failing: ODM completes with exit 0 but logsCannot compute alignment matrix/Alignment will be skipped, and noregistration.jsonis produced.Bisecting release images against the same dataset I found that the error happened between 3.3.4 and 3.5.1 (relatively large range due to the docker images available, but there weren't too many that touched codem or align).
In 3.0.4 which is the first available image with the
--alignflag, the codem feature matching debug image (opensfm/stats/codem/dsm_feature_matches.png) looked like this:In latest it looks like this:
In a70e7445 the default
--feature-typechanged fromsifttodspsift. That doesn't touch the alignment code but it does change the reconstruction, whose DSM is what codem tries to match against the reference.This does fix the test, but rather than stop there, I wanted to understand why it was failing with
dspsift. As I understand it, the--alignflag is meant to align a new output with a secondary source - either an old run or even something third party? In the oats case, the brighton dataset has a generateddsm.tifandmodel.lazfile that we use as the base to align to. These were generated with ~v3.0.2. When these were generated, we almost guarantee a match as we are using the same version to generate both the reference and comparison. However over time the output has drifted (i.e. getting more accurate), but the change todspsiftwas the final nail where the reconstruction's DSM was different enough that this made it too hard for codem to find correct matches.If this is the case, switching back to
siftisn't necessarily the right move, it would just be a fix to the test, not the product. We could just update the datasets generated files, but that would effectively be the same as disabling the test.Instead I had a look to see if we could make the feature matching more stable.
One thing that stood out to me is that the 3.5.1 and 3.5.6 releases fail due to a rejected scale being found. This seemed to be caused by bad feature matches. I also noted that the "correct" scale is actually always 1.0. This makes sense when we realise both sides of the match are georeferenced metric data in the same CRS. To test this, I removed the scale as a solving parameter in codem which effectively locked the scale to 1. This removes a degree of freedom from the solver which, in theory, means there might be fewer matches, but the matches should be higher quality.
Notice that the images are almost exactly the same as the failing case, but the features are matched much better. This is very similar to the
siftfix above, but this time the scale is a perfect 1.0 (as we fixed it). Thesiftfix while looking correct, actually had a scale of roughly 0.8 and so would have resized the model/outputs by 20%.One last thing, the
dsm.tifin the brighton dataset actually has a different (wrong?) CSM "UTM zone 10N". This didn't seem to affect the bug though it might affect other outputs. Themodel.lazas a CSM of "UTM zone 15N" which I believe is correct but still failed above.I believe this is the correct fix, but I would appreciate more experienced eyes on it @smathermather.