fix vectorlayer 3D entity generation by filtering duplicates #52064

benoitdm-oslandia · 2023-03-01T17:24:35Z

This PR follow #53063

The feature selection, in QgsVectorLayerChunkLoader, was made according to the intersection of the current quadtree node and the feature geometry. For all features with geometry across quadtree nodes, the geometries were duplicated inducing memory and cpu degradations. This fix uses the previous filter (bbox intersection) then filters according to the geometry centroid.

closes #50971

Geometry / entity

Old version

A feature is included in multiple quadtree nodes. We use 1064 primitives in 18 geometries.
If we export the scene and import it into Blender we obtain 14 full objects (11 duplicates):

New version

Features across multiple quadtree nodes are clipped in the node bbox. We use 838 primitives in 18 geometries.

If we export the scene and import it into Blender we obtain 14 partial objects (one per quadtree node). The only duplicated faces are those created by the walls when the geometry is clipped:

Computation time

To do this test, I use a bigger project with 57000 features (from a Geopackage) with QGIS built in release.

I add a QElapsedTimer to monitor the time (I will remove it before the merge).

Old version

Log extract:


new 3D view map

Warning: Logged warning: ========================== job timer restarted!
Warning: Logged warning: Job took: 81ms
Warning: Logged warning: Job took: 228ms
Warning: Logged warning: Job took: 3317ms

new 3D view map

Warning: Logged warning: ========================== job timer restarted!
Warning: Logged warning: Job took: 38ms
Warning: Logged warning: Job took: 58ms
Warning: Logged warning: Job took: 90ms
Warning: Logged warning: Job took: 2205ms

new 3D view map

Warning: Logged warning: ========================== job timer restarted!
Warning: Logged warning: Job took: 17ms
Warning: Logged warning: Job took: 65ms
Warning: Logged warning: Job took: 2895ms

new 3D view map

Warning: Logged warning: ========================== job timer restarted!
Warning: Logged warning: Job took: 9ms
Warning: Logged warning: Job took: 30ms
Warning: Logged warning: Job took: 95ms
Warning: Logged warning: Job took: 2327ms

(3317+2205+2895+2327)÷4 ~= 2686ms for 3672934 primitives ==> 0,73µs per primitives.

New version

Log extract:

new 3D view map

Warning: Logged warning: ========================== job timer restarted!
Warning: Logged warning: Job took: 88ms
Warning: Logged warning: Job took: 182ms
Warning: Logged warning: Job took: 3116ms

new 3D view map

Warning: Logged warning: ========================== job timer restarted!
Warning: Logged warning: Job took: 9ms
Warning: Logged warning: Job took: 42ms
Warning: Logged warning: Job took: 79ms
Warning: Logged warning: Job took: 2120ms

new 3D view map

Warning: Logged warning: ========================== job timer restarted!
Warning: Logged warning: Job took: 25ms
Warning: Logged warning: Job took: 29ms
Warning: Logged warning: Job took: 36ms
Warning: Logged warning: Job took: 2775ms

new 3D view map

Warning: Logged warning: ========================== job timer restarted!
Warning: Logged warning: Job took: 22ms
Warning: Logged warning: Job took: 43ms
Warning: Logged warning: Job took: 71ms
Warning: Logged warning: Job took: 2170ms

(3116+2120+2775+2170)÷4 ~= 2545ms ==> for 3299502 primitives ==> 0,77µs per primitive

We lost ~5% in speed and we gain ~11,3% in primitive count.

The main gain is we have no more geometry duplicates.

nyalldawson · 2023-03-01T17:52:49Z

feature is included in only one quadtree node

Wouldn't this mean that features will become invisible as soon as the node containing their centroid out of view? (Even if other nodes containing part of the geometry are still visible)

benoitdm-oslandia · 2023-03-02T07:15:02Z

Wouldn't this mean that features will become invisible as soon as the node containing their centroid out of view? (Even if other nodes containing part of the geometry are still visible)

Yes, exactly!

nyalldawson · 2023-03-02T18:04:08Z

@benoitdm-oslandia isn't that a regression? Objects will disappear unexpectedly from the edge of views as a 3d map is navigated....

uclaros · 2023-03-06T07:56:22Z

@benoitdm-oslandia also, keep in mind to perform any benchmarking on the optimized release build, as the debug build can have significantly different results!

benoitdm-oslandia · 2023-03-10T17:49:47Z

You were right! It was more obvious for concave geometries.

I rework my PR by clipping the 2D polygon with the bbox of the current chunk node. Only the used part is kept in the chunk node entity. There is no more duplicate but the clipping increase by a little (17µs per geom) the rendering time.

I will push some perf stats on monday.

github-actions · 2023-03-30T02:41:26Z

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

that all unit tests are passing
that all comments by reviewers have been addressed
that there is enough information for reviewers, in particular
- link to any issues which this pull request fixes
- add a description of workflows which this pull request fixes
- add screenshots if applicable
that you have written unit tests where possible
In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
If there is no further activity on this pull request, it will be closed in a week.

benoitdm-oslandia · 2023-03-30T07:23:22Z

@uclaros @nyalldawson can you please have a look?

uclaros · 2023-03-30T21:34:12Z

I'm not sure I like the idea of clipping on the chunk extents. The seams will be visible when rendering edges is enabled.
If the only problem with the currently duplicated primitives is exporting, then would it be possible to implement some fid based filtering when exporting (I've no clue about the exporting code) to avoid exporting the same fid twice?

On another note, I love the tidying up and setting of objectNames! That will help debugging and exploring the scene in Gammaray!
I'd suggest you wrap those unrelated fixes in a separate PR

benoitdm-oslandia · 2023-03-31T07:25:12Z

I'm not sure I like the idea of clipping on the chunk extents. The seams will be visible when rendering edges is enabled.

Yes indeed.

If the only problem with the currently duplicated primitives is exporting, then would it be possible to implement some fid based filtering when exporting (I've no clue about the exporting code) to avoid exporting the same fid twice?

From the exporting function pov the only known objects are the QEntity objects obtained by the createEntity function of a chunkLoader. These Qt entities do not have any fid field.

solution would be to create a base object Qgs3DFeatureEntity (inheriting QEntity) and change every chunkLoader logic to handle the new object and assign fid.
solution would be to use the composition design already available to the QEntity and add a new component (with fid) to each QEntity. But we will need to change every chunkLoader logic to handle the component.
solution would be to add a dynamic property to the QEntity with the fid and again to change every chunkLoader logic to handle the property.

IMHO the 2. and 3. solution are prone to error due to the need to think (or to forget) to add the property or the component. 1. solution in another hand, by changing the chunkLoader signature can force to add the fid.

But this is a big change just to filter objects during export.

wonder-sk · 2023-03-31T11:03:54Z

One more issue with the current clipping approach is that it is likely not going to work with polygons that are not horizontal (this is common with city models, for example with roofs) - as far as I know, GEOS only does the operations in 2D.

By the way, in the PR, expected_polygon3d_extrusion_opacity.png seems to have a visible horizontal shift compared to the original image - is that correct?

The de-duplication of features in export should be doable with the current implementation - for the purposes of the identify tool, we keep a data structure to understand what triangles map to what feature ID (for tessellated polygon geometries), so this data structure could be used for de-duplication too.

A proper solution would be nice to have for the 3D view too - for solid objects it's all fine, but semi-transparent objects on tile boundaries may be drawn more solid due to the duplication. Apart from the original approach with one feature being only in one tile, and the current approach with clipping, I can imagine a third approach - entities for tiles would only contain features fully inside, and the features spanning more than one tile would be rendered on special "shared" entities which would be enabled whenever adjacent entity gets enabled. That approach would solve all the issues: edge highlighting, semi-transparent objects and export.

benoitdm-oslandia · 2023-09-06T12:43:36Z

please @nyalldawson, @lbartoletti or @troopa81, can I have a review?

benoitdm-oslandia · 2023-09-07T11:58:19Z

@nyalldawson @uclaros can you review please?

src/3d/qgs3dexportobject.cpp

src/3d/symbols/qgspolygon3dsymbol.cpp

src/3d/qgsvectorlayerchunkloader_p.cpp

src/3d/qgstessellatedpolygongeometry.h

src/3d/qgs3dsceneexporter.cpp

nyalldawson · 2023-09-08T00:30:41Z

Just some very minor changes from my end, otherwise all looks good!

benoitdm-oslandia · 2023-09-08T07:36:57Z

@nyalldawson I have squashed the history too. Let me know!

…inct empty and data entities

…he same function. Were created in one function and deleted from another.

…StartingIndices accessors

uclaros

Just a few consts here and there

tests/src/3d/testqgs3drendering.cpp

src/3d/symbols/qgspolygon3dsymbol.cpp

src/3d/qgs3dsceneexporter.cpp

…ries

…llocation by uniq ptr

…ndexes are a subset of all vertice

…e scene * keep memory of exported features * compute polygon triangle subsets matching the feature to export

…ltering for 3d scene export

…me and avoid the use mFactory->mLayer->name()

benoitdm-oslandia · 2023-09-12T05:55:14Z

thanks @nyalldawson @uclaros!

nyalldawson · 2023-09-14T05:01:33Z

@benoitdm-oslandia looks like the new test is a bit fragile -- see eg https://github.com/qgis/QGIS/actions/runs/6179574806/job/16777801856?pr=54578 for a random failure it's been triggering

benoitdm-oslandia · 2023-09-14T07:43:53Z

Train trip today! I will try to fix that asap

benoitdm-oslandia requested a review from vcloarec March 1, 2023 17:24

github-actions bot added this to the 3.30.0 milestone Mar 1, 2023

benoitdm-oslandia requested a review from uclaros March 1, 2023 17:24

benoitdm-oslandia self-assigned this Mar 1, 2023

benoitdm-oslandia requested a review from nyalldawson March 1, 2023 17:24

benoitdm-oslandia added the 3D Relates to QGIS' 3D engine or rendering label Mar 1, 2023

benoitdm-oslandia force-pushed the fix/50971-export branch from 7081cb7 to 2a9ba5c Compare March 1, 2023 17:27

benoitdm-oslandia marked this pull request as draft March 2, 2023 07:16

nyalldawson modified the milestones: 3.30.0, 3.32 (feature) Mar 6, 2023

YoannQDQ modified the milestones: 3.32 (feature), 3.32.0 Mar 6, 2023

benoitdm-oslandia force-pushed the fix/50971-export branch from 2a9ba5c to 5facc7a Compare March 10, 2023 17:46

benoitdm-oslandia force-pushed the fix/50971-export branch 3 times, most recently from 69632bf to a0e3ffc Compare March 14, 2023 16:28

benoitdm-oslandia marked this pull request as ready for review March 15, 2023 08:15

github-actions bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Mar 30, 2023

benoitdm-oslandia removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Mar 30, 2023

github-actions bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Apr 15, 2023

benoitdm-oslandia force-pushed the fix/50971-export branch 2 times, most recently from 3e3ac5d to 0ca88ca Compare September 5, 2023 12:22

nyalldawson reviewed Sep 8, 2023

View reviewed changes

benoitdm-oslandia force-pushed the fix/50971-export branch from 0ca88ca to 7d4c85d Compare September 8, 2023 07:37

benoitdm-oslandia added 3 commits September 11, 2023 09:23

fix(QgsVectorLayerChunkLoader): set name to Qt3DCore::QEntity to dist…

3898537

…inct empty and data entities

fix(QgsPolygon3DSymbolHandler): polygon are cloned and destroyed by t…

3dfe337

…he same function. Were created in one function and deleted from another.

feat(QgsTessellatedPolygonGeometry): add featureIds and triangleIndex…

0a1a7e4

…StartingIndices accessors

benoitdm-oslandia force-pushed the fix/50971-export branch from 14adf84 to 4c484ec Compare September 11, 2023 07:23

uclaros reviewed Sep 11, 2023

View reviewed changes

benoitdm-oslandia added 6 commits September 11, 2023 14:21

fix(QgsPolygon3DSymbol): rename var and improve check in exportGeomet…

0dc7320

…ries

fix(QgsPolygon3DSymbol_p): remove C style casts and replace pointer a…

49d24ac

…llocation by uniq ptr

feat(Qgs3DExportObject): improve obj export to support when vertice i…

841ab4b

…ndexes are a subset of all vertice

Feat(Qgs3DSceneExporter): remove polygon duplicates when exporting th…

6b3035e

…e scene * keep memory of exported features * compute polygon triangle subsets matching the feature to export

feat(Qgs3DSceneExporter): add test to validate feature duplication fi…

68c7d65

…ltering for 3d scene export

fix(QgsVectorLayerChunkLoader): create field member to store layer na…

ce2522f

…me and avoid the use mFactory->mLayer->name()

benoitdm-oslandia force-pushed the fix/50971-export branch from 4c484ec to ce2522f Compare September 11, 2023 12:22

uclaros approved these changes Sep 11, 2023

View reviewed changes

benoitdm-oslandia removed the request for review from vcloarec September 11, 2023 13:16

nyalldawson approved these changes Sep 11, 2023

View reviewed changes

nyalldawson merged commit 1cf6dfc into qgis:master Sep 11, 2023
28 checks passed

benoitdm-oslandia deleted the fix/50971-export branch September 12, 2023 05:54

benoitdm-oslandia mentioned this pull request Sep 18, 2023

fix(3DSceneExporter): add entity sorting to have idempotent scene export #54649

Merged

ptitjano mentioned this pull request Jun 10, 2024

Exporting a 3D scene results in a broken obj file #57437

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix vectorlayer 3D entity generation by filtering duplicates #52064

fix vectorlayer 3D entity generation by filtering duplicates #52064

benoitdm-oslandia commented Mar 1, 2023 •

edited

Loading

nyalldawson commented Mar 1, 2023

benoitdm-oslandia commented Mar 2, 2023

nyalldawson commented Mar 2, 2023

uclaros commented Mar 6, 2023

benoitdm-oslandia commented Mar 10, 2023

github-actions bot commented Mar 30, 2023

benoitdm-oslandia commented Mar 30, 2023

uclaros commented Mar 30, 2023

benoitdm-oslandia commented Mar 31, 2023

wonder-sk commented Mar 31, 2023

benoitdm-oslandia commented Sep 6, 2023

benoitdm-oslandia commented Sep 7, 2023

nyalldawson commented Sep 8, 2023

benoitdm-oslandia commented Sep 8, 2023

uclaros left a comment

benoitdm-oslandia commented Sep 12, 2023

nyalldawson commented Sep 14, 2023

benoitdm-oslandia commented Sep 14, 2023

fix vectorlayer 3D entity generation by filtering duplicates #52064

fix vectorlayer 3D entity generation by filtering duplicates #52064

Conversation

benoitdm-oslandia commented Mar 1, 2023 • edited Loading

Geometry / entity

Old version

New version

Computation time

Old version

New version

nyalldawson commented Mar 1, 2023

benoitdm-oslandia commented Mar 2, 2023

nyalldawson commented Mar 2, 2023

uclaros commented Mar 6, 2023

benoitdm-oslandia commented Mar 10, 2023

github-actions bot commented Mar 30, 2023

benoitdm-oslandia commented Mar 30, 2023

uclaros commented Mar 30, 2023

benoitdm-oslandia commented Mar 31, 2023

wonder-sk commented Mar 31, 2023

benoitdm-oslandia commented Sep 6, 2023

benoitdm-oslandia commented Sep 7, 2023

nyalldawson commented Sep 8, 2023

benoitdm-oslandia commented Sep 8, 2023

uclaros left a comment

Choose a reason for hiding this comment

benoitdm-oslandia commented Sep 12, 2023

nyalldawson commented Sep 14, 2023

benoitdm-oslandia commented Sep 14, 2023

benoitdm-oslandia commented Mar 1, 2023 •

edited

Loading