Jump to conversation
Unresolved conversations (4)
@nyalldawson nyalldawson Aug 22, 2019
Does this have ownership? If so, it should be a unique_ptr
Outdated
src/3d/symbols/qgspoint3dsymbol.h
ismailsunni
@nyalldawson nyalldawson Aug 22, 2019
What happens to ownership here? Looks like texture2D may be leaked
Outdated
...d/symbols/qgspoint3dbillboardmaterial.cpp
ismailsunni
@nyalldawson nyalldawson Aug 22, 2019
defaultSymbol is leaked here
Outdated
...d/symbols/qgspoint3dbillboardmaterial.cpp
ismailsunni
@nyalldawson nyalldawson Aug 22, 2019
Is this a factory?
...generated/symbols/qgspoint3dsymbol.sip.in
Resolved conversations (27)
@wonder-sk wonder-sk Aug 25, 2019
can we have different color for the symbol outline from the scene background color? Now it is unclear in the rendering output what is still part of the billboard and what is already the background...
tests/src/3d/testqgs3drendering.cpp
@wonder-sk wonder-sk Aug 25, 2019
probably we don't need this #include of XML utils here anymore
Outdated
src/app/3d/qgspoint3dsymbolwidget.cpp
@wonder-sk wonder-sk Aug 25, 2019
probably we don't need this #include of XML utils here anymore
Outdated
src/3d/symbols/qgspoint3dsymbol_p.cpp
@wonder-sk wonder-sk Aug 25, 2019
The comment should say that the ownership of the symbol object is transferred
src/3d/symbols/qgspoint3dsymbol.h
@wonder-sk wonder-sk Aug 25, 2019
Do we still need this constructor? If not better to remove it
Outdated
src/3d/symbols/qgspoint3dsymbol.h
ismailsunni
@wonder-sk wonder-sk Aug 25, 2019
Ummm.... this can be simplified - the returned symbolElem could be simply attached to DOM (without saving the XML to a text stream).
Outdated
src/3d/symbols/qgspoint3dsymbol.cpp
@wonder-sk wonder-sk Aug 25, 2019
answer to Nyall's question is yes - it does take ownership if the image does not have a parent yet - it would be good to have a comment here that the image gets parented to texture2D so it is clear...
...d/symbols/qgspoint3dbillboardmaterial.cpp
@wonder-sk wonder-sk Aug 25, 2019
no memory leak here anymore :)
Outdated
...d/symbols/qgspoint3dbillboardmaterial.cpp
@wonder-sk wonder-sk Aug 25, 2019
"dots per inch"
Outdated
src/3d/qgs3dmapsettings.h
@wonder-sk wonder-sk Aug 22, 2019
It is worth noting that the 'selected' parameter was added in 3.10
src/core/symbology/qgssymbol.h
@wonder-sk wonder-sk Aug 22, 2019
there should be no need to set unit scale and no rotation to the matrix - that should be the default already.
Outdated
src/3d/symbols/qgspoint3dsymbol.cpp
@wonder-sk wonder-sk Aug 22, 2019
is there any reason for this line? maybe some leftover from debugging?
Outdated
src/3d/symbols/qgspoint3dsymbol_p.cpp
ismailsunni
@wonder-sk wonder-sk Aug 22, 2019
use "context" that is passed to readXml() method instead of creating an empty context - otherwise relative paths would not get resolved correctly to absolute paths
Outdated
src/3d/symbols/qgspoint3dsymbol.cpp
@wonder-sk wonder-sk Aug 22, 2019
this code should use QgsSymbolLayerUtils::saveSymbol() - and the current read/write context should be passed to it so that any paths can be converted to relative.
Outdated
src/3d/symbols/qgspoint3dsymbol.cpp
@wonder-sk wonder-sk Aug 22, 2019
we can make this method private - I guess client code should use the higher level calls (setTexture2DFrom...()) or actually its body could be moved to setTexture2DFromTextureImage() as that's the only place where it's used and it's single line only...
Outdated
...d/symbols/qgspoint3dbillboardmaterial.cpp
ismailsunni
@wonder-sk wonder-sk Aug 22, 2019
Is this method used anywhere? if not we can remove it...
Outdated
...d/symbols/qgspoint3dbillboardmaterial.cpp
ismailsunni
@nyalldawson nyalldawson Aug 22, 2019
```suggestion * \param set to TRUE to render the symbol in a selected state ```
Outdated
src/core/symbology/qgssymbollayerutils.h
@nyalldawson nyalldawson Aug 22, 2019
Possible ownership issue here, depending on whether or not setBillboardSymbol takes ownership or internally makes a clone. (Ideally it should take ownership and follow the patterns from other parts of QGIS). In which case, this should use QgsSymbolButton::clonedSymbol instead.
Outdated
src/app/3d/qgspoint3dsymbolwidget.cpp
ismailsunni
@nyalldawson nyalldawson Aug 22, 2019
remove
Outdated
src/app/3d/qgspoint3dsymbolwidget.cpp
@nyalldawson nyalldawson Aug 22, 2019
Does this take ownership?
...d/symbols/qgspoint3dbillboardmaterial.cpp
@nyalldawson nyalldawson Aug 22, 2019
This is leaked -- create it on the stack, not the heap
Outdated
...d/symbols/qgspoint3dbillboardmaterial.cpp
@nyalldawson nyalldawson Aug 22, 2019
const?
Outdated
src/3d/symbols/qgsbillboardgeometry.h
@nyalldawson nyalldawson Aug 22, 2019
Does this take ownership?
...generated/symbols/qgspoint3dsymbol.sip.in
@PeterPetrik PeterPetrik Aug 20, 2019
copy-paste error
...d/symbols/qgspoint3dbillboardmaterial.cpp
@PeterPetrik PeterPetrik Aug 20, 2019
wrong header (copy-paste error)
Outdated
src/3d/symbols/qgsbillboardgeometry.cpp
@PeterPetrik PeterPetrik Aug 20, 2019
please remove commented out code
Outdated
src/3d/shaders/billboards.geom
@PeterPetrik PeterPetrik Aug 20, 2019
please remove commented out code
Outdated
src/3d/shaders/billboards.frag