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] Billboard Rendering for Point 3D #31308

Merged
merged 73 commits into from Aug 27, 2019

Conversation

@ismailsunni
Copy link
Contributor

ismailsunni commented Aug 20, 2019

Description

This PR is related to GSoC 2019 Project: QGIS 3D Improvement. It implements the billboard rendering for 3d points.. The functionalities:

  • Render points as a billboard in 3D map view.
  • Set billboard from symbol (with all options in symbol: size, color, rotation, etc)
  • Set the height of the billboard

Screencast: https://youtu.be/GZSfgSvJwZY

Checklist

  • Commit messages are descriptive and explain the rationale for changes
  • Commits which fix bugs include Fixes #11111 at the bottom of the commit message
  • I have read the QGIS Coding Standards and this PR complies with them
  • New unit tests have been added for core changes
  • I have run the scripts/prepare-commit.sh script before each commit
  • I have evaluated whether it is appropriate for this PR to be backported, backport requests are left as label or comment
@@ -96,6 +97,15 @@ Returns a key-value dictionary of point shape properties
void setShapeProperties( const QVariantMap &properties );
%Docstring
Sets a key-value dictionary of point shape properties
%End

QgsMarkerSymbol *billboardSymbol() const;

This comment has been minimized.

Copy link
@nyalldawson

nyalldawson Aug 22, 2019

Contributor

Is this a factory?

src/3d/symbols/qgsbillboardgeometry.h Outdated Show resolved Hide resolved
src/3d/symbols/qgspoint3dbillboardmaterial.cpp Outdated Show resolved Hide resolved
// Default texture
QgsMarkerSymbol *defaultSymbol = static_cast<QgsMarkerSymbol *>( QgsSymbol::defaultSymbol( QgsWkbTypes::PointGeometry ) );

setTexture2DFromSymbol( defaultSymbol, map, selected );

This comment has been minimized.

Copy link
@nyalldawson

nyalldawson Aug 22, 2019

Contributor

defaultSymbol is leaked here

This comment has been minimized.

Copy link
@ismailsunni

ismailsunni Aug 23, 2019

Author Contributor

Change to use unique_ptr to solve this.


void QgsPoint3DBillboardMaterial::setTexture2D( Qt3DRender::QTexture2D *texture2D )
{
mTexture2D->setValue( QVariant::fromValue( texture2D ) );

This comment has been minimized.

Copy link
@nyalldawson

nyalldawson Aug 22, 2019

Contributor

What happens to ownership here? Looks like texture2D may be leaked

This comment has been minimized.

Copy link
@ismailsunni

ismailsunni Aug 23, 2019

Author Contributor

Move the function's body to setTexture2DFromTextureImage. I will discuss with @wonder-sk tomorrow for the detail of this ownership and memory leak.

@@ -96,6 +106,7 @@ class _3D_EXPORT QgsPoint3DSymbol : public QgsAbstract3DSymbol
Shape mShape = Cylinder; //!< What kind of shape to use
QVariantMap mShapeProperties; //!< Key-value dictionary of shape's properties (different keys for each shape)
QMatrix4x4 mTransform; //!< Transform of individual instanced models
QgsMarkerSymbol *mBillboardSymbol = nullptr;

This comment has been minimized.

Copy link
@nyalldawson

nyalldawson Aug 22, 2019

Contributor

Does this have ownership? If so, it should be a unique_ptr

This comment has been minimized.

Copy link
@ismailsunni

ismailsunni Aug 23, 2019

Author Contributor

Changed to use unique_ptr. I need to re-implement the clone function to do this. I am not quite sure if my approach is good or not.

src/app/3d/qgspoint3dsymbolwidget.cpp Outdated Show resolved Hide resolved
src/app/3d/qgspoint3dsymbolwidget.cpp Outdated Show resolved Hide resolved
src/core/symbology/qgssymbollayerutils.h Outdated Show resolved Hide resolved
src/3d/symbols/qgspoint3dbillboardmaterial.cpp Outdated Show resolved Hide resolved
src/3d/symbols/qgspoint3dbillboardmaterial.cpp Outdated Show resolved Hide resolved
src/3d/symbols/qgspoint3dsymbol.cpp Outdated Show resolved Hide resolved
src/3d/symbols/qgspoint3dsymbol.cpp Outdated Show resolved Hide resolved
src/3d/symbols/qgspoint3dsymbol_p.cpp Outdated Show resolved Hide resolved
src/3d/symbols/qgspoint3dsymbol.cpp Outdated Show resolved Hide resolved
src/core/symbology/qgssymbol.h Show resolved Hide resolved
Copy link
Member

wonder-sk left a comment

Looking great.

I still have problem with the billboard symbol button not shown - I need to change the .ui's file layout from form to grid in order to get the button displayed :-/

src/3d/qgs3dmapsettings.h Outdated Show resolved Hide resolved
src/3d/symbols/qgspoint3dbillboardmaterial.cpp Outdated Show resolved Hide resolved
src/3d/symbols/qgspoint3dsymbol.cpp Outdated Show resolved Hide resolved
src/3d/symbols/qgspoint3dsymbol.h Outdated Show resolved Hide resolved
src/3d/symbols/qgspoint3dsymbol.h Show resolved Hide resolved
src/3d/symbols/qgspoint3dsymbol_p.cpp Outdated Show resolved Hide resolved
src/app/3d/qgspoint3dsymbolwidget.cpp Outdated Show resolved Hide resolved
tests/src/3d/testqgs3drendering.cpp Show resolved Hide resolved
@wonder-sk wonder-sk merged commit b9978d5 into qgis:master Aug 27, 2019
1 check failed
1 check failed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.