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.

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

Pointcloud improvement #746

Merged
merged 2 commits into from
Apr 30, 2018
Merged

Pointcloud improvement #746

merged 2 commits into from
Apr 30, 2018

Conversation

peppsac
Copy link
Contributor

@peppsac peppsac commented Apr 27, 2018

(reopening #732 )

2 commits improving pointcloud support for itowns.

The first one fixes the point budget handling for .bin format.

The second reworks SSE: it's similar to #674 but simpler and only for pointclouds.

Full commits message:

  • refactor(pointcloud): rework point budget implementation
Since .bin and .cin have different repartitions of the points inside a node,
they must use different algorithms.
For .cin we want to draw a %-age of each node because points are evenly
distributed.
For .bin, we sort the nodes by 'importance', and stop drawing nodes when we
have enoug points.
  • refactor(pointcloud): simplify SSE computation for pointcloud
Project average point spacing on screen and compare to sse threshold.
It's a simplification of the approach proposed in #674.

This allows to drop a dependency (convexhull) and brings the SSE
computation for pointcloud closer to what is done for other geometries.

Edit:
The SSE value computed here is the estimation of the maximum distance between 2 points in a given node.
PotreeConverter guarantees that the distance between each pair of points is >= metadata.spacing / node.level in all nodes. This is easy to visualize if you display successive children (using the 'sticky' debug tool):
distances

The SSE is the estimation of this distance projected on the screen.

Since .bin and .cin have different repartitions of the points inside a node,
they must use different algorithms.
For .cin we want to draw a %-age of each node because points are evenly
distributed.
For .bin, we sort the nodes by 'importance', and stop drawing nodes when we
have enoug points.
@peppsac peppsac mentioned this pull request Apr 27, 2018
Copy link
Contributor

@autra autra left a comment

Choose a reason for hiding this comment

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

I just want to understand the change https -> http in the package-lock.json. Seems very strange to me.

@@ -5954,7 +5034,7 @@
},
"onetime": {
"version": "1.1.0",
"resolved": "https://registry.npmjs.org/onetime/-/onetime-1.1.0.tgz",
"resolved": "http://registry.npmjs.org/onetime/-/onetime-1.1.0.tgz",
Copy link
Contributor

Choose a reason for hiding this comment

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

huh?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No idea where this change comes from.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm investigating

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you try to run again npm i in your branch to see if package-lock.json changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope, doesn't change

Copy link
Contributor

Choose a reason for hiding this comment

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

I've pushed a commit with a package-lock.json generated with npm 6. Can you try if your npm i still works ok? if so, we can use this one I guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It works, but npm i reverts all your changes to package-lock.json, eg:

-            "find-up": "^1.0.0",
-            "read-pkg": "^1.0.0"
+            "find-up": "1.1.2",
+            "read-pkg": "1.1.0"

I guess it's expected since I don't use npm 6...

}
} else if (!elt.promise) {
const distance = Math.max(0.001, bbox.distanceToPoint(context.camera.camera3D.position));
// Increase priority of nearest node
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand the "nearest" here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nearest = closest to the camera.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh! right... I find the comment more confusing than the code alone :-)

} else if (!elt.promise) {
const distance = Math.max(0.001, bbox.distanceToPoint(context.camera.camera3D.position));
// Increase priority of nearest node
const priority = computeScreenSpaceError(context, layer, elt, distance) / distance;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't you set elt.sse here? You'd have it already on line 217 this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could but that would complexify a bit the code at line 127 (with a if (elt.sse is already computed)) for something that only happens once per node: when we decide to fetch them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be guaranteed to be computed on line 127? (because elt.promise would be undefined the first time update is called on this node). But even if so, I agree this is a bit fragile. We can keep it like that.

Project average point spacing on screen and compare to sse threshold.
It's a simplification of the approach proposed in #674.

This allows to drop a dependency (convexhull) and brings the SSE
compuation for pointcloud closer to what is done for other geometries.
@peppsac peppsac force-pushed the pointcloud_improvment branch from 131bd25 to 89b6cbf Compare April 30, 2018 11:52
@autra
Copy link
Contributor

autra commented Apr 30, 2018

thanks!

@autra autra merged commit f18548f into master Apr 30, 2018
@autra autra deleted the pointcloud_improvment branch April 30, 2018 12:55
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.

2 participants