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.

Sign up for GitHub

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

Self Masking for Vector Layers #584

Merged
merged 3 commits into from
Feb 1, 2024
Merged

Self Masking for Vector Layers #584

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0f1b758
Add support for self-masking vector layers
maurhofer-ubique Jan 23, 2024
ae51246
Keep order of polygon/line geometries in vector layers, don't overrid…
maurhofer-ubique Jan 24, 2024
0d43778
adds iOS part
zimmermannubique Feb 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions android/src/main/cpp/graphics/objects/LineGroup2dOpenGl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,11 +109,7 @@ void LineGroup2dOpenGl::render(const std::shared_ptr<::RenderingContextInterface

std::shared_ptr<OpenGlContext> openGlContext = std::static_pointer_cast<OpenGlContext>(context);
if (isMasked) {
if (isMaskInversed) {
glStencilFunc(GL_EQUAL, 0, 255);
} else {
glStencilFunc(GL_EQUAL, 128, 255);
}
glStencilFunc(GL_EQUAL, isMaskInversed ? 0 : 128, 255);
} else {
glEnable(GL_STENCIL_TEST);
glStencilMask(0xFF);
Expand Down
21 changes: 15 additions & 6 deletions android/src/main/cpp/graphics/objects/Polygon2dOpenGl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,14 +104,23 @@ void Polygon2dOpenGl::render(const std::shared_ptr<::RenderingContextInterface>

std::shared_ptr<OpenGlContext> openGlContext = std::static_pointer_cast<OpenGlContext>(context);

GLuint stencilMask = 0;
GLuint validTarget = 0;
GLenum zpass = GL_KEEP;
if (isMasked) {
if (isMaskInversed) {
glStencilFunc(GL_EQUAL, 0, 255);
} else {
glStencilFunc(GL_EQUAL, 128, 255);
}
stencilMask += 128;
validTarget = isMaskInversed ? 0 : 128;
}
glStencilOp(GL_KEEP, GL_KEEP, GL_INCR);
if (renderPass.isPassMasked) {
stencilMask += 127;
zpass = GL_INCR;
}

if (stencilMask != 0) {
glStencilFunc(GL_EQUAL, validTarget, stencilMask);
glStencilOp(GL_KEEP, GL_KEEP, zpass);
Comment on lines +107 to +121
Copy link
Contributor

Choose a reason for hiding this comment

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

The modifications to the render method for handling stencil operations based on masking conditions are well-intended for enhancing rendering flexibility. However, consider the following:

  • The logic for calculating stencilMask and validTarget should be thoroughly tested to ensure it behaves as expected under all combinations of isMasked and renderPass.isPassMasked.
  • The choice of 128 and 127 for modifying stencilMask values appears arbitrary without context. Ensure these values are documented to explain their significance and how they contribute to the intended stencil operations.
  • Using GL_INCR for zpass assumes a specific usage pattern of the stencil buffer. Confirm that this approach is compatible with the overall rendering strategy and does not risk stencil buffer overflow, which could lead to rendering issues.

Adding detailed comments to explain the logic behind the chosen values and operations would greatly aid in the maintainability of this code. Additionally, consider edge cases where the combined stencil operations might not produce the expected results and adjust or document as necessary.

}

glColorMask(GL_TRUE, GL_TRUE, GL_TRUE, GL_TRUE);

drawPolygon(openGlContext, program, mvpMatrix);
Expand Down
16 changes: 14 additions & 2 deletions android/src/main/cpp/graphics/objects/PolygonGroup2dOpenGl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,9 +98,21 @@ void PolygonGroup2dOpenGl::render(const std::shared_ptr<::RenderingContextInterf
if (!ready)
return;

GLuint stencilMask = 0;
GLuint validTarget = 0;
GLenum zpass = GL_KEEP;
if (isMasked) {
glStencilFunc(GL_EQUAL, isMaskInversed ? 0 : 128, 128);
glStencilOp(GL_KEEP, GL_KEEP, GL_KEEP);
stencilMask += 128;
validTarget = isMaskInversed ? 0 : 128;
}
if (renderPass.isPassMasked) {
stencilMask += 127;
zpass = GL_INCR;
}

if (stencilMask != 0) {
glStencilFunc(GL_EQUAL, validTarget, stencilMask);
glStencilOp(GL_KEEP, GL_KEEP, zpass);
Comment on lines +101 to +115
Copy link
Contributor

Choose a reason for hiding this comment

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

The implementation of stencil operations in the render method introduces a more flexible handling of masking conditions. However, consider the following points for improvement:

  • Ensure that the initial values of stencilMask and validTarget are appropriately chosen for all possible states of isMasked and renderPass.isPassMasked.
  • The increment of stencilMask by 127 when renderPass.isPassMasked is true might lead to unexpected behavior if both isMasked and renderPass.isPassMasked are true, as the total value could exceed the maximum value representable in the stencil buffer. Verify that this logic aligns with the intended stencil operations.
  • The use of GL_INCR for zpass when renderPass.isPassMasked is true assumes that the stencil buffer is being used in a way that supports incremental updates. Ensure this matches the overall rendering strategy and that there's no risk of overflow in the stencil values which could lead to visual artifacts.

Consider adding comments to explain the rationale behind the chosen stencil operation values, especially the significance of 128 and 127, to improve maintainability and readability for future developers.

}

std::shared_ptr<OpenGlContext> openGlContext = std::static_pointer_cast<OpenGlContext>(context);
Expand Down
16 changes: 14 additions & 2 deletions android/src/main/cpp/graphics/objects/PolygonPatternGroup2dOpenGl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,9 +141,21 @@ void PolygonPatternGroup2dOpenGl::render(const std::shared_ptr<::RenderingContex

glUseProgram(program);

GLuint stencilMask = 0;
GLuint validTarget = 0;
GLenum zpass = GL_KEEP;
if (isMasked) {
glStencilFunc(GL_EQUAL, isMaskInversed ? 0 : 128, 128);
glStencilOp(GL_KEEP, GL_KEEP, GL_KEEP);
stencilMask += 128;
validTarget = isMaskInversed ? 0 : 128;
}
if (renderPass.isPassMasked) {
stencilMask += 127;
zpass = GL_INCR;
}

if (stencilMask != 0) {
glStencilFunc(GL_EQUAL, validTarget, stencilMask);
glStencilOp(GL_KEEP, GL_KEEP, zpass);
}

prepareTextureDraw(program);
Expand Down
16 changes: 14 additions & 2 deletions android/src/main/cpp/graphics/objects/Quad2dInstancedOpenGl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,9 +206,21 @@ void Quad2dInstancedOpenGl::render(const std::shared_ptr<::RenderingContextInter

glUseProgram(program);

GLuint stencilMask = 0;
GLuint validTarget = 0;
GLenum zpass = GL_KEEP;
if (isMasked) {
glStencilFunc(GL_EQUAL, isMaskInversed ? 0 : 128, 128);
glStencilOp(GL_KEEP, GL_KEEP, GL_KEEP);
stencilMask += 128;
validTarget = isMaskInversed ? 0 : 128;
}
if (renderPass.isPassMasked) {
stencilMask += 127;
zpass = GL_INCR;
}

if (stencilMask != 0) {
glStencilFunc(GL_EQUAL, validTarget, stencilMask);
glStencilOp(GL_KEEP, GL_KEEP, zpass);
}

if (usesTextureCoords) {
Expand Down
16 changes: 14 additions & 2 deletions android/src/main/cpp/graphics/objects/Quad2dOpenGl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,9 +193,21 @@ void Quad2dOpenGl::render(const std::shared_ptr<::RenderingContextInterface> &co

glUseProgram(program);

GLuint stencilMask = 0;
GLuint validTarget = 0;
GLenum zpass = GL_KEEP;
if (isMasked) {
glStencilFunc(GL_EQUAL, isMaskInversed ? 0 : 128, 128);
glStencilOp(GL_KEEP, GL_KEEP, GL_KEEP);
stencilMask += 128;
validTarget = isMaskInversed ? 0 : 128;
}
if (renderPass.isPassMasked) {
stencilMask += 127;
zpass = GL_INCR;
}

if (stencilMask != 0) {
glStencilFunc(GL_EQUAL, validTarget, stencilMask);
glStencilOp(GL_KEEP, GL_KEEP, zpass);
Comment on lines +196 to +210
Copy link
Contributor

Choose a reason for hiding this comment

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

The modifications to the render method in Quad2dOpenGl for handling stencil operations based on masking conditions are well-implemented. However, ensure that:

  • The logic for calculating stencilMask, validTarget, and zpass is thoroughly tested, especially considering the potential for both isMasked and renderPass.isPassMasked to be true simultaneously.
  • The values 128 and 127 used for stencilMask adjustments are clearly documented, explaining their significance and ensuring that they do not inadvertently exceed the stencil buffer's capacity.
  • The choice of GL_INCR for zpass is consistent with the overall rendering strategy and does not lead to stencil buffer overflow, which could cause rendering artifacts.

Adding comments to explain the rationale behind the chosen stencil operation values and ensuring that the implementation is robust against all possible combinations of masking conditions would improve code clarity and maintainability.

}

if (usesTextureCoords) {
Expand Down
16 changes: 14 additions & 2 deletions android/src/main/cpp/graphics/objects/Quad2dStretchedInstancedOpenGl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,9 +205,21 @@ void Quad2dStretchedInstancedOpenGl::render(const std::shared_ptr<::RenderingCon

glUseProgram(program);

GLuint stencilMask = 0;
GLuint validTarget = 0;
GLenum zpass = GL_KEEP;
if (isMasked) {
glStencilFunc(GL_EQUAL, isMaskInversed ? 0 : 128, 128);
glStencilOp(GL_KEEP, GL_KEEP, GL_KEEP);
stencilMask += 128;
validTarget = isMaskInversed ? 0 : 128;
}
if (renderPass.isPassMasked) {
stencilMask += 127;
zpass = GL_INCR;
}

if (stencilMask != 0) {
glStencilFunc(GL_EQUAL, validTarget, stencilMask);
glStencilOp(GL_KEEP, GL_KEEP, zpass);
}

if (usesTextureCoords) {
Expand Down
16 changes: 14 additions & 2 deletions android/src/main/cpp/graphics/objects/Text2dInstancedOpenGl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,9 +200,21 @@ void Text2dInstancedOpenGl::render(const std::shared_ptr<::RenderingContextInter

glUseProgram(program);

GLuint stencilMask = 0;
GLuint validTarget = 0;
GLenum zpass = GL_KEEP;
if (isMasked) {
glStencilFunc(GL_EQUAL, isMaskInversed ? 0 : 128, 128);
glStencilOp(GL_KEEP, GL_KEEP, GL_KEEP);
stencilMask += 128;
validTarget = isMaskInversed ? 0 : 128;
}
if (renderPass.isPassMasked) {
stencilMask += 127;
zpass = GL_INCR;
}

if (stencilMask != 0) {
glStencilFunc(GL_EQUAL, validTarget, stencilMask);
glStencilOp(GL_KEEP, GL_KEEP, zpass);
}

if (usesTextureCoords) {
Expand Down
16 changes: 14 additions & 2 deletions android/src/main/cpp/graphics/objects/Text2dOpenGl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,9 +209,21 @@ void Text2dOpenGl::render(const std::shared_ptr<::RenderingContextInterface> &co
return;
}

GLuint stencilMask = 0;
GLuint validTarget = 0;
GLenum zpass = GL_KEEP;
if (isMasked) {
glStencilFunc(GL_EQUAL, isMaskInversed ? 0 : 128, 128);
glStencilOp(GL_KEEP, GL_KEEP, GL_KEEP);
stencilMask += 128;
validTarget = isMaskInversed ? 0 : 128;
}
if (renderPass.isPassMasked) {
stencilMask += 127;
zpass = GL_INCR;
}

if (stencilMask != 0) {
glStencilFunc(GL_EQUAL, validTarget, stencilMask);
glStencilOp(GL_KEEP, GL_KEEP, zpass);
}

glUseProgram(program);
Expand Down
1 change: 1 addition & 0 deletions bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/RenderPassConfig.kt
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 5 additions & 3 deletions bridging/android/jni/graphics/NativeRenderPassConfig.cpp
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion bridging/android/jni/graphics/NativeRenderPassConfig.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 4 additions & 2 deletions bridging/ios/MCRenderPassConfig+Private.mm
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 6 additions & 2 deletions bridging/ios/MCRenderPassConfig.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 6 additions & 2 deletions bridging/ios/MCRenderPassConfig.mm
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions djinni/graphics/core.djinni
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ render_pass_interface = interface +c +j +o {

render_pass_config = record {
render_pass_index: i32;
is_pass_masked: bool;
}

renderer_interface = interface +c +j +o {
Expand Down
2 changes: 1 addition & 1 deletion shared/public/BackgroundVectorLayerDescription.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ class BackgroundVectorLayerDescription: public VectorLayerDescription {
BackgroundVectorStyle style,
std::optional<int32_t> renderPassIndex,
std::shared_ptr<Value> interactable):
VectorLayerDescription(identifier, "", "", 0, 0, nullptr, renderPassIndex, interactable, false),
VectorLayerDescription(identifier, "", "", 0, 0, nullptr, renderPassIndex, interactable, false, false),
style(style) {};

std::unique_ptr<VectorLayerDescription> clone() override {
Expand Down
7 changes: 4 additions & 3 deletions shared/public/LineVectorLayerDescription.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,14 +134,15 @@ class LineVectorLayerDescription: public VectorLayerDescription {
LineVectorStyle style,
std::optional<int32_t> renderPassIndex,
std::shared_ptr<Value> interactable,
bool multiselect):
VectorLayerDescription(identifier, source, sourceId, minZoom, maxZoom, filter, renderPassIndex, interactable, multiselect),
bool multiselect,
bool selfMasked):
VectorLayerDescription(identifier, source, sourceId, minZoom, maxZoom, filter, renderPassIndex, interactable, multiselect, selfMasked),
style(style) {};

std::unique_ptr<VectorLayerDescription> clone() override {
return std::make_unique<LineVectorLayerDescription>(identifier, source, sourceLayer, minZoom, maxZoom,
filter ? filter->clone() : nullptr, style, renderPassIndex,
interactable ? interactable->clone() : nullptr, multiselect);
interactable ? interactable->clone() : nullptr, multiselect, selfMasked);
Comment on lines +137 to +145
Copy link
Contributor

Choose a reason for hiding this comment

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

The addition of the selfMasked boolean parameter to the LineVectorLayerDescription constructor and its inclusion in the clone method is a significant enhancement, allowing for more detailed control over layer rendering properties. Ensure that:

  • The selfMasked parameter is correctly documented in the class header to explain its purpose and how it affects rendering behavior.
  • All usages of LineVectorLayerDescription throughout the codebase have been updated to account for the new parameter, ensuring that existing functionality is not inadvertently affected.

Consider implementing default values for the selfMasked parameter in scenarios where backward compatibility with existing code might be necessary. Additionally, unit tests should be updated or added to cover the new functionality introduced by this parameter.

}

virtual UsedKeysCollection getUsedKeys() const override {
Expand Down
7 changes: 4 additions & 3 deletions shared/public/PolygonVectorLayerDescription.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,14 +99,15 @@ class PolygonVectorLayerDescription: public VectorLayerDescription {
PolygonVectorStyle style,
std::optional<int32_t> renderPassIndex,
std::shared_ptr<Value> interactable,
bool multiselect):
VectorLayerDescription(identifier, source, sourceId, minZoom, maxZoom, filter, renderPassIndex, interactable, multiselect),
bool multiselect,
bool selfMasked):
VectorLayerDescription(identifier, source, sourceId, minZoom, maxZoom, filter, renderPassIndex, interactable, multiselect, selfMasked),
style(style) {};

std::unique_ptr<VectorLayerDescription> clone() override {
return std::make_unique<PolygonVectorLayerDescription>(identifier, source, sourceLayer, minZoom, maxZoom,
filter ? filter->clone() : nullptr, style, renderPassIndex,
interactable ? interactable->clone() : nullptr, multiselect);
interactable ? interactable->clone() : nullptr, multiselect, selfMasked);
}

virtual UsedKeysCollection getUsedKeys() const override {
Expand Down
2 changes: 1 addition & 1 deletion shared/public/RasterVectorLayerDescription.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ class RasterVectorLayerDescription: public VectorLayerDescription {
bool underzoom,
bool overzoom,
std::optional<::RectCoord> bounds):
VectorLayerDescription(identifier, source, "", minZoom, maxZoom, nullptr, renderPassIndex, interactable, false),
VectorLayerDescription(identifier, source, "", minZoom, maxZoom, nullptr, renderPassIndex, interactable, false, false),
Copy link
Contributor

Choose a reason for hiding this comment

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

The introduction of an additional boolean parameter in the RasterVectorLayerDescription constructor to initialize VectorLayerDescription with false, false for the last two arguments is a notable change. Ensure that:

  • The purpose of these boolean parameters, especially the newly added one, is clearly documented in both the RasterVectorLayerDescription and VectorLayerDescription class headers. This will help developers understand their impact on layer behavior.
  • All instantiations of RasterVectorLayerDescription throughout the codebase have been reviewed to ensure that the correct values are being passed, maintaining the intended functionality.

Adding explicit documentation for the new parameter and its expected behavior would enhance code readability and maintainability. Consider reviewing related classes or methods that might be affected by this change to ensure consistency across the codebase.

style(style), url(url), underzoom(underzoom), overzoom(overzoom), adaptScaleToScreen(adaptScaleToScreen), numDrawPreviousLayers(numDrawPreviousLayers),
maskTiles(maskTiles), zoomLevelScaleFactor(zoomLevelScaleFactor), bounds(bounds) {};

Expand Down
6 changes: 4 additions & 2 deletions shared/public/RenderPassConfig.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,11 @@

struct RenderPassConfig final {
int32_t renderPassIndex;
bool isPassMasked;
Copy link
Contributor

Choose a reason for hiding this comment

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

The use of std::move for primitive types like int32_t and bool is unnecessary and does not have any effect.

-    : renderPassIndex(std::move(renderPassIndex_))
-    , isPassMasked(std::move(isPassMasked_))
+    : renderPassIndex(renderPassIndex_)
+    , isPassMasked(isPassMasked_)
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
bool isPassMasked;
bool isPassMasked;


//NOLINTNEXTLINE(google-explicit-constructor)
RenderPassConfig(int32_t renderPassIndex_)
RenderPassConfig(int32_t renderPassIndex_,
bool isPassMasked_)
: renderPassIndex(std::move(renderPassIndex_))
, isPassMasked(std::move(isPassMasked_))
{}
};
Loading