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

[3D] Add support for data defined properties of points #57929

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

ptitjano
Copy link
Contributor

Description

This PR introduces support for data defined properties (radius, length...). This needs some changes in QgsInstancedPoint3DSymbolHandler. Inded, it uses OpenGL instancing in order to draw multiple times the same entity at different locations with the same shape. In practice, this means that only 2 Qt3D entities are
created:

  • 1 for the points which are not selected
  • 1 for the selected points
    Indeed, the selected points do not have the same color as the normal ones. If there isn't any selected point, only one entity is created.

When a point can have a data defined shapes, this logic needs to be updated. The idea is to group the points which have the same shape. This logic is handled by the PointData class and the == operator. If two different have the same state (selected or normal) and the same shape, they are grouped together in the same PointData
instance and their respective positions are stored in positions. With that logic, the following entities are created:

  1. Entity1: (normal, radius1, length1)
  2. Entity2: (selected, radius1, length1)
  3. Entity3: (normal, radius1, lenght2)
  4. Entity4: (selected, radius1, lenght2)
  5. Entity5: (normal, radius2, lenght1)
  6. Entity6: (selected, radius2, lenght1)
  7. Entity5: (normal, radius2, lenght2)
  8. Entity6: (selected, radius2, lenght2)
    ...

If there is only one shape, the previous case still applies.

Example

Capture d’écran du 2024-06-28 16-05-33

Capture d’écran du 2024-06-28 16-06-06

@github-actions github-actions bot added this to the 3.40.0 milestone Jun 28, 2024
@ptitjano ptitjano added Feature 3D Relates to QGIS' 3D engine or rendering labels Jun 28, 2024
@ptitjano ptitjano self-assigned this Jun 28, 2024
`QgsInstancedPoint3DSymbolHandler` uses OpenGL instancing in order to
draw multiple times the same entity at different locations with the
same shape. In practice, this means that only two Qt3D entities are
created:
- one for the points which are not selected
- one for the selected points
Indeed, the selected points do not have the same color as the normal
ones. If there isn't any selected point, only one entity is created.

When a point can have a data defined radius, this logic needs to be
updated. The idea is to group the points which have the same
radius. This logic is handled by the `PointData` class and the `==`
operator. If two different have the same state (selected or normal)
and the same radius, they are grouped together in the same `PointData`
instance and their respective positions are stored in
`positions`. This means that if there are 3 different radii, at most 6
different entities are created:
1. Entity1: (normal, radius1)
2. Entity2: (selected, radius1)
3. Entity3: (normal, radius2)
4. Entity4: (selected, radius2)
5. Entity5: (normal, radius3)
6. Entity6: (selected, radius3)

If there is only one radius, the previous case still applies.
This will be used by `QgsPoint3DSymbol` in the next commit.
This will be used by `QgsPoint3DSymbol` in the next commit.
This will be used by `QgsPoint3DSymbol` in the next commit.
This will be used by `QgsPoint3DSymbol` in the next commit.
@ptitjano ptitjano force-pushed the data-defined-point-3d-part1 branch from 171a697 to db63491 Compare June 28, 2024 14:23
src/3d/symbols/qgspoint3dsymbol_p.cpp Show resolved Hide resolved
src/3d/symbols/qgspoint3dsymbol_p.cpp Outdated Show resolved Hide resolved
BottomRadius SIP_MONKEYPATCH_COMPAT_NAME( PropertyHeight ), //!< Bottom Radius
MinorRadius SIP_MONKEYPATCH_COMPAT_NAME( PropertySize ), //!< Minor Radius
TopRadius SIP_MONKEYPATCH_COMPAT_NAME( PropertyHeight ), //!< Top Radius
Size SIP_MONKEYPATCH_COMPAT_NAME( PropertySize ), //!< Size
Copy link
Contributor

Choose a reason for hiding this comment

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

Size is not a reserved word?

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 don't know.

Copy link

github-actions bot commented Jun 28, 2024

🪟 Windows builds ready!

Windows builds of this PR are available for testing here. Debug symbols for this build are available here.

(Built from commit c5cc462)

@nyalldawson
Copy link
Collaborator

I'm wondering how well this approach scales. Eg if you had a point layer with thousands of points , and each has there own distinct height, we'll end up with a LOT of entities. Have you tested to see what the usable limits are? I suspect we would need some way to abort creating new entities early if we approach the usable limit...

Copy link

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.

@github-actions github-actions bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Jul 13, 2024
@wonder-sk
Copy link
Member

I have the same concerns as Nyall - this approach will likely not perform well when used with slightly larger datasets, with many thousands of draw commands required. The correct solution here is to add data-defined properties to vertex buffers and handle sizing in the vertex shader code...

@github-actions github-actions bot removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Jul 18, 2024
Copy link

github-actions bot commented Aug 2, 2024

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.

@github-actions github-actions bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Aug 2, 2024
@ptitjano ptitjano removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Aug 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3D Relates to QGIS' 3D engine or rendering Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants