Skip to content
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

Fix introduction of decimal errors due to double decimal precision using BROTLI #611

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open

Fix introduction of decimal errors due to double decimal precision using BROTLI #611

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
17cf961
Fix introduction of decimal errors due to double decimal precision
Feb 20, 2023
14ccb47
Fix bounding box check rounding error
Feb 28, 2023
3e7eee9
Also round bounding box check, otherwise still false positives on poi…
Mar 2, 2023
0f9f309
Merge branch 'develop' of https://github.com/potree/PotreeConverter i…
Mar 29, 2023
74fa74f
Parallel sort has a memory leak in linux. Use sequential sort.
Aug 13, 2024
2e4dd85
Merge branch 'develop' of https://github.com/tobias93/PotreeConverter…
Sep 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions Converter/include/sampler_poisson.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,8 +179,7 @@ struct SamplerPoisson : public Sampler {

};

auto parallel = std::execution::par_unseq;
std::sort(parallel, points.begin(), points.end(), [center](Point a, Point b) -> bool {
std::sort(points.begin(), points.end(), [center](Point a, Point b) -> bool {

auto ax = a.x - center.x;
auto ay = a.y - center.y;
Expand Down
3 changes: 1 addition & 2 deletions Converter/include/sampler_poisson_average.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,8 +280,7 @@ struct SamplerPoissonAverage : public Sampler {

};

auto parallel = std::execution::par_unseq;
std::sort(parallel, points.begin(), points.end(), [center](Point a, Point b) -> bool {
std::sort(points.begin(), points.end(), [center](Point a, Point b) -> bool {

auto ax = a.x - center.x;
auto ay = a.y - center.y;
Expand Down
44 changes: 27 additions & 17 deletions Converter/src/chunker_countsort_laszip.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,9 +225,9 @@ namespace chunker_countsort_laszip {
double y = coordinates[1];
double z = coordinates[2];

int32_t X = int32_t((x - posOffset.x) / posScale.x);
int32_t Y = int32_t((y - posOffset.y) / posScale.y);
int32_t Z = int32_t((z - posOffset.z) / posScale.z);
int32_t X = int32_t(std::round((x - posOffset.x) / posScale.x));
int32_t Y = int32_t(std::round((y - posOffset.y) / posScale.y));
int32_t Z = int32_t(std::round((z - posOffset.z) / posScale.z));

double ux = (double(X) * posScale.x + posOffset.x - min.x) / size.x;
double uy = (double(Y) * posScale.y + posOffset.y - min.y) / size.y;
Expand All @@ -237,17 +237,27 @@ namespace chunker_countsort_laszip {
inBox = inBox && ux <= 1.0 && uy <= 1.0 && uz <= 1.0;

if (!inBox) {
stringstream ss;
ss << "encountered point outside bounding box." << endl;
ss << "box.min: " << min.toString() << endl;
ss << "box.max: " << max.toString() << endl;
ss << "point: " << Vector3(x, y, z).toString() << endl;
ss << "file: " << path << endl;
ss << "PotreeConverter requires a valid bounding box to operate." << endl;
ss << "Please try to repair the bounding box, e.g. using lasinfo with the -repair_bb argument." << endl;
logger::ERROR(ss.str());

exit(123);
// Also check with unrounded bounding box

bool xInBox = std::round(x / posScale.x) >= std::round(min.x / posScale.x) && std::round(x / posScale.x) <= std::round((min.x + size.x) / posScale.x);
bool yInBox = std::round(y / posScale.y) >= std::round(min.y / posScale.y) && std::round(y / posScale.y) <= std::round((min.y + size.y) / posScale.y);
bool zInBox = std::round(z / posScale.z) >= std::round(min.z / posScale.z) && std::round(z / posScale.z) <= std::round((min.z + size.z) / posScale.z);

if (!(xInBox && yInBox && zInBox))
{
// Point truely outside bounding box
stringstream ss;
ss << "encountered point outside bounding box." << endl;
ss << "box.min: " << min.toString() << endl;
ss << "box.max: " << max.toString() << endl;
ss << "point: " << Vector3(x, y, z).toString() << endl;
ss << "file: " << path << endl;
ss << "PotreeConverter requires a valid bounding box to operate." << endl;
ss << "Please try to repair the bounding box, e.g. using lasinfo with the -repair_bb argument." << endl;
logger::ERROR(ss.str());

exit(123);
}
}

int64_t ix = int64_t(std::min(dGridSize * ux, dGridSize - 1.0));
Expand Down Expand Up @@ -778,9 +788,9 @@ namespace chunker_countsort_laszip {
double y = coordinates[1];
double z = coordinates[2];

int32_t X = int32_t((x - outputAttributes.posOffset.x) / scale.x);
int32_t Y = int32_t((y - outputAttributes.posOffset.y) / scale.y);
int32_t Z = int32_t((z - outputAttributes.posOffset.z) / scale.z);
int32_t X = int32_t(std::round((x - outputAttributes.posOffset.x) / scale.x));
int32_t Y = int32_t(std::round((y - outputAttributes.posOffset.y) / scale.y));
int32_t Z = int32_t(std::round((z - outputAttributes.posOffset.z) / scale.z));

memcpy(data + offset + 0, &X, 4);
memcpy(data + offset + 4, &Y, 4);
Expand Down
6 changes: 3 additions & 3 deletions Converter/src/indexer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1162,9 +1162,9 @@ SoA toStructOfArrays(Node* node, Attributes attributes) {
};
vector<P> ps;
P min;
min.x = std::numeric_limits<int64_t>::max();
min.y = std::numeric_limits<int64_t>::max();
min.z = std::numeric_limits<int64_t>::max();
min.x = 0;
min.y = 0;
min.z = 0;
Comment on lines +1165 to +1167
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi, can you tell me the reason for initializing min values with 0? As far as I can tell that would make the bounding box minimum 0, even if it is actually a large integer value.

Since min values are int32_t, shouldn't they be initialized with min.x = std::numeric_limits<int32_t>::max(); (instead of int64_t that it was before.

Copy link
Author

Choose a reason for hiding this comment

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

To be honest this was trial and error.

I had tried what you suggested but then the whole result got scrambled. Like the higher the node level the more points were pulled towards the min xy of the complete dataset. Maybe because the viewer build upon this bug in decoding the Morton code.

Then I tried setting it 0 and the problem of coordinate differences got a little bit less.

The true solution was all the rounding that I introduced in stead of casting to int32_t.

Copy link
Contributor

Choose a reason for hiding this comment

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

The original initialization appears to be doing this:
min = { -1, -1, -1 };
...as the result from int64_t max value conversions to int32_t.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps this min struct could just be a constant, or totally eliminated. Point X, Y, Z values are relative to octree bounding box min, and always >= 0, are they not?

Copy link
Author

Choose a reason for hiding this comment

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

Yes I think you're right. Just left it in there as Markus might want to throw in some other min values. But until now it was -1, -1, -1 indeed.


for (int64_t i = 0; i < numPoints; i++) {

Expand Down