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

Feature/offscreen render targets #683

Merged
merged 6 commits into from
Jan 21, 2025

Conversation

maerki
Copy link
Contributor

@maerki maerki commented Jan 21, 2025

Summary by CodeRabbit

Release Notes

  • New Features

    • Added support for render targets across multiple rendering interfaces
    • Introduced offscreen rendering capabilities
    • Enhanced rendering flexibility with optional render target parameters
  • Improvements

    • Updated drawing methods to accept optional render targets
    • Added ability to check for computation requirements
    • Expanded layer and scene rendering interfaces
  • Technical Enhancements

    • Introduced RenderTargetInterface for more granular rendering control
    • Modified render pass construction to support render targets
    • Updated bridging code for Android, iOS, and cross-platform compatibility

Choose a reason for hiding this comment

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

Copilot reviewed 55 out of 75 changed files in this pull request and generated no comments.

Files not reviewed (20)
  • bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/RenderPassInterface.kt: Language not supported
  • bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/RenderTargetInterface.kt: Language not supported
  • bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/RendererInterface.kt: Language not supported
  • bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/SceneInterface.kt: Language not supported
  • bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/common/Color.kt: Language not supported
  • bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/common/Quad2dD.kt: Language not supported
  • bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/common/Quad3dD.kt: Language not supported
  • bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/common/Vec2D.kt: Language not supported
  • bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/common/Vec2F.kt: Language not supported
  • bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/common/Vec3D.kt: Language not supported
  • bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/shader/RasterShaderStyle.kt: Language not supported
  • bridging/android/java/io/openmobilemaps/mapscore/shared/map/LayerInterface.kt: Language not supported
  • bridging/android/java/io/openmobilemaps/mapscore/shared/map/MapInterface.kt: Language not supported
  • bridging/android/java/io/openmobilemaps/mapscore/shared/map/coordinates/Coord.kt: Language not supported
  • bridging/android/java/io/openmobilemaps/mapscore/shared/map/coordinates/QuadCoord.kt: Language not supported
  • bridging/android/java/io/openmobilemaps/mapscore/shared/map/coordinates/RectCoord.kt: Language not supported
  • bridging/android/java/io/openmobilemaps/mapscore/shared/map/layers/text/FormattedStringEntry.kt: Language not supported
  • bridging/android/jni/graphics/NativeRenderPassInterface.cpp: Language not supported
  • bridging/android/jni/graphics/NativeRenderPassInterface.h: Language not supported
  • bridging/android/jni/graphics/NativeRenderTargetInterface.cpp: Language not supported
Copy link
Contributor

coderabbitai bot commented Jan 21, 2025

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

This pull request introduces a comprehensive enhancement to the rendering pipeline across multiple platforms and languages. The primary focus is the addition of a new RenderTargetInterface and modifications to various rendering-related interfaces and implementations. The changes enable more flexible rendering by allowing components to specify optional render targets, supporting offscreen rendering, and providing methods to check computation requirements. These modifications span Android, iOS, and shared C++ codebases, ensuring consistent behavior across different platforms.

Changes

File/Group Change Summary
Render Interfaces Added RenderTargetInterface with methods to get/set render targets in multiple interfaces
Layer Classes Updated to support optional render targets in render pass generation and layer methods
Renderer/Scene Classes Modified drawFrame methods to accept optional render targets
Platform Bridging Added JNI, Objective-C, and Swift implementations for new render target functionality
Djinni Definitions Updated interface definitions to include render target support

Sequence Diagram

sequenceDiagram
    participant MapView
    participant Scene
    participant Renderer
    participant Layer
    participant RenderPass
    participant RenderTarget

    MapView->>Scene: drawFrame(target)
    Scene->>Renderer: drawFrame(context, camera, target)
    Renderer->>Layer: buildRenderPasses(target)
    Layer->>RenderPass: create(config, objects, mask, target)
    RenderPass-->>Renderer: render passes
    Renderer->>RenderTarget: prepare rendering
    Renderer->>RenderTarget: execute rendering
Loading

Poem

🎨 Render Rabbit's Rhapsody 🐰

With targets new and render bright,
Our pixels dance in pure delight,
Offscreen magic, layers gleam,
A rendering computational dream!

Hop, hop, hooray for flexible frames! 🌈


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 7

🔭 Outside diff range comments (2)
bridging/android/java/io/openmobilemaps/mapscore/shared/map/layers/text/FormattedStringEntry.kt (1)

Line range hint 1-1: Modifications to auto-generated code should be made in the source definition.

This file is auto-generated from text.djinni as indicated by the header comment. While initializing tempResult to 0 is correct, making direct modifications to this file is risky as they will be overwritten during the next code generation.

Instead of modifying this file directly:

  1. Update the Djinni definition in text.djinni
  2. Regenerate the code using the Djinni tool
  3. If this is a Djinni template issue, consider fixing it at the template level to ensure consistent initialization across all generated comparison methods

Also applies to: 12-12

bridging/android/java/io/openmobilemaps/mapscore/shared/map/coordinates/QuadCoord.kt (1)

Line range hint 1-1: ⚠️ This file should not be modified directly as it is autogenerated.

The file header explicitly states "AUTOGENERATED FILE - DO NOT MODIFY!". Any direct modifications to this file will be lost when the code is regenerated from the Djinni definition file.

Instead of modifying this file directly, update the source definition in coordinate_system.djinni and regenerate the code.

🧹 Nitpick comments (18)
bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/common/Vec2D.kt (1)

12-12: LGTM! Consider optimization in the template.

The initialization of tempResult to 0 is technically redundant since the first if-else block always assigns a value. However, this change aligns with similar modifications in other vector/color classes and improves code clarity.

In the source template, you could potentially optimize this by removing the initialization since tempResult is always set in the first if-else block.

bridging/android/java/io/openmobilemaps/mapscore/shared/map/coordinates/Coord.kt (1)

18-18: Consider optimizing the comparison logic.

While the initialization of tempResult to 0 is functionally correct, it's redundant since:

  1. Each comparison block already sets tempResult to 0 in the else clause
  2. The method returns early if tempResult != 0
  3. The final return is 0

Consider simplifying the comparison logic in the Djinni source file. Here's a more concise approach:

override fun compareTo(other: Coord): Int {
-    var tempResult = 0
-    if (this.systemIdentifier < other.systemIdentifier) {
-        tempResult = -1;
-    } else if (this.systemIdentifier > other.systemIdentifier) {
-        tempResult = 1;
-    } else {
-        tempResult = 0;
-    }
-    if (tempResult != 0) {
-        return tempResult
-    }
+    systemIdentifier.compareTo(other.systemIdentifier).let { if (it != 0) return it }
+    x.compareTo(other.x).let { if (it != 0) return it }
+    y.compareTo(other.y).let { if (it != 0) return it }
+    return z.compareTo(other.z)
ios/maps/Layers/Layer.swift (1)

14-14: Add documentation for the beforeAdding property.

Please add documentation comments explaining:

  • The purpose of this callback
  • When it gets called in the layer lifecycle
  • The expected usage patterns

Example:

/// Callback executed just before the layer is added to a MapView.
/// - Parameters:
///   - interface: The layer interface being added
///   - mapView: The target MapView
var beforeAdding: ((MCLayerInterface, MCMapView) -> Void)? { get }
ios/maps/Layers/RasterLayer.swift (1)

23-31: Document the beforeAdding parameter in convenience init.

Add parameter documentation to match the comprehensive style used for other parameters.

/// - Parameters:
///   - layerName: Identifier for layer
///   - urlFormat: URL for tile with placeholders, e.g. https://www.sample.org/{z}/{x}/{y}.png
///   - layerIndex: Optional index for layer ordering
///   - beforeAdding: Optional callback executed just before the layer is added to a MapView
ios/maps/Layers/VectorLayer.swift (2)

18-19: Document the beforeAdding parameter.

Add parameter documentation to match other parameters in this initializer.


33-34: Consider grouping related properties together.

The beforeAdding property could be grouped with other layer-related properties (interface, layerIndex) for better organization.

    public var layerIndex: Int?
+    public var beforeAdding: ((MCLayerInterface, MCMapView) -> Void)?

    public let selectionHandler = MCTiled2dMapVectorLayerSelectionCallbackHandler()
-    public var beforeAdding: ((MCLayerInterface, MCMapView) -> Void)?
bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/common/Color.kt (1)

14-14: Consider the necessity of initializing tempResult.

While initializing tempResult to 0 is safer than leaving it uninitialized, in this case it's redundant because:

  1. The first if-else block always assigns a value before any usage
  2. The variable is scoped within the method and reassigned in each comparison block
  3. The Kotlin compiler ensures initialization safety

Since this is an autogenerated file, propose this optimization in the source common.djinni file instead:

-        var tempResult = 0
+        var tempResult: Int
shared/src/graphics/Renderer.h (1)

33-33: LGTM! Consider adding documentation for the new parameter.

The addition of the nullable render target parameter is well-structured and consistent with the interface changes. Consider adding a brief documentation comment explaining the purpose and behavior when target is null vs. non-null.

    /** Ensure calling on graphics thread */
    void drawFrame(const std::shared_ptr<RenderingContextInterface> &renderingContext,
-                   const std::shared_ptr<CameraInterface> &camera, const /*nullable*/ std::shared_ptr<RenderTargetInterface> & target) override;
+                   const std::shared_ptr<CameraInterface> &camera,
+                   /// @param target When non-null, renders to the specified target instead of the default framebuffer
+                   const /*nullable*/ std::shared_ptr<RenderTargetInterface> & target) override;
shared/public/SimpleLayerInterface.h (2)

18-20: Consider documenting the renderTarget member variable.

While the protected member variable is appropriately placed, adding documentation would help derived classes understand its purpose and lifecycle.

protected:
+   /// The primary render target for this layer. When null, renders to the default framebuffer
    std::shared_ptr<::RenderTargetInterface> renderTarget;

34-36: LGTM! Consider adding override requirement.

The virtual method implementation is correct, but consider requiring derived classes to call the base implementation when overriding.

Add a comment like:

    /// @note Derived classes must call the base implementation when overriding
    virtual void setPrimaryRenderTarget(const /*nullable*/ std::shared_ptr<::RenderTargetInterface> & target) {
        renderTarget = target;
    }
ios/maps/RenderTargetTexture.swift (2)

1-15: Remove duplicate file header.

The file contains duplicate copyright headers. Remove one of them to maintain cleaner code.

-//
-//  RenderTargetTexture.swift
-//  MapCore
-//
-//  Created by Nicolas Märki on 15.01.2025.
-//
-
-
-//
-//  RenderTargetTexture.swift
-//
-//
-//  Created by Nicolas Märki on 28.09.22.
-//
+//
+//  RenderTargetTexture.swift
+//  MapCore
+//
+//  Created by Nicolas Märki on 15.01.2025.
+//

22-25: Add documentation for the textures array structure.

The tuple structure in the textures array would benefit from documentation explaining the purpose of each texture type (color and stencilDepth).

 open class RenderTargetTexture: Identifiable, Equatable, MCRenderTargetInterface {
     public let name: String
 
+    /// Array of texture pairs used for offscreen rendering.
+    /// - color: The main color texture for rendering the content
+    /// - stencilDepth: The stencil texture for masking operations
     private var textures: [(color: MTLTexture, stencilDepth: MTLTexture)] = []
djinni/graphics/core.djinni (1)

63-65: Document the purpose of the empty interface.

The empty render_target_interface should include documentation explaining its purpose and expected implementations across different platforms.

+# Interface for platform-specific render target implementations.
+# iOS: Implemented by RenderTargetTexture using Metal textures
+# Android: Implemented using OpenGL framebuffers
 render_target_interface = interface +c +j +o {
 
 }
shared/public/MapInterface.h (1)

82-82: LGTM! Consider adding documentation.

The getNeedsCompute() method is a good addition for optimization purposes. Consider adding documentation to explain when it returns true/false and how it fits into the rendering lifecycle.

+    /** Returns true if the map needs to be recomputed before the next frame can be rendered. */
     virtual bool getNeedsCompute() = 0;
djinni/map/core.djinni (1)

62-65: Consider caching computation needs state.

The new methods get_needs_compute() and draw_offscreen_frame() suggest frequent offscreen rendering capabilities. Consider implementing a caching mechanism for the computation needs state to optimize performance.

bridging/android/java/io/openmobilemaps/mapscore/shared/map/MapInterface.kt (2)

65-66: Add documentation for the new method.

Please add KDoc documentation explaining when computation is needed and how this method should be used in the rendering pipeline.

+    /** Returns true if the map needs to perform compute operations before rendering. */
     abstract fun getNeedsCompute(): Boolean

67-68: Add documentation for offscreen rendering method.

Please add KDoc documentation explaining the purpose of offscreen rendering and how to properly use the render target parameter.

+    /**
+     * Renders a frame to the specified offscreen render target.
+     * @param target The render target to draw to.
+     */
     abstract fun drawOffscreenFrame(target: io.openmobilemaps.mapscore.shared.graphics.RenderTargetInterface)
ios/maps/MCMapView.swift (1)

42-43: Consider using a dictionary for render target lookup.

The current implementation uses linear search to find render targets by name. For better performance, consider using a dictionary with the target name as the key.

-    public var renderTargetTextures: [RenderTargetTexture] = []
+    private var renderTargetTexturesDict: [String: RenderTargetTexture] = [:]
+    public var renderTargetTextures: [RenderTargetTexture] {
+        Array(renderTargetTexturesDict.values)
+    }

     public func renderTarget(named name: String) -> RenderTargetTexture {
-        for target in renderTargetTextures {
-            if target.name == name {
-                return target
-            }
-        }
-        let newTarget = RenderTargetTexture(name: name)
-        renderTargetTextures.append(newTarget)
-        return newTarget
+        if let target = renderTargetTexturesDict[name] {
+            return target
+        }
+        let newTarget = RenderTargetTexture(name: name)
+        renderTargetTexturesDict[name] = newTarget
+        return newTarget
     }

Also applies to: 170-179

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c9a90c2 and 05703ef.

📒 Files selected for processing (75)
  • bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/RenderPassInterface.kt (2 hunks)
  • bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/RenderTargetInterface.kt (1 hunks)
  • bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/RendererInterface.kt (2 hunks)
  • bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/SceneInterface.kt (2 hunks)
  • bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/common/Color.kt (1 hunks)
  • bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/common/Quad2dD.kt (1 hunks)
  • bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/common/Quad3dD.kt (1 hunks)
  • bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/common/Vec2D.kt (1 hunks)
  • bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/common/Vec2F.kt (1 hunks)
  • bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/common/Vec3D.kt (1 hunks)
  • bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/shader/RasterShaderStyle.kt (1 hunks)
  • bridging/android/java/io/openmobilemaps/mapscore/shared/map/LayerInterface.kt (2 hunks)
  • bridging/android/java/io/openmobilemaps/mapscore/shared/map/MapInterface.kt (2 hunks)
  • bridging/android/java/io/openmobilemaps/mapscore/shared/map/coordinates/Coord.kt (1 hunks)
  • bridging/android/java/io/openmobilemaps/mapscore/shared/map/coordinates/QuadCoord.kt (1 hunks)
  • bridging/android/java/io/openmobilemaps/mapscore/shared/map/coordinates/RectCoord.kt (1 hunks)
  • bridging/android/java/io/openmobilemaps/mapscore/shared/map/layers/text/FormattedStringEntry.kt (1 hunks)
  • bridging/android/jni/graphics/NativeRenderPassInterface.cpp (3 hunks)
  • bridging/android/jni/graphics/NativeRenderPassInterface.h (2 hunks)
  • bridging/android/jni/graphics/NativeRenderTargetInterface.cpp (1 hunks)
  • bridging/android/jni/graphics/NativeRenderTargetInterface.h (1 hunks)
  • bridging/android/jni/graphics/NativeRendererInterface.cpp (3 hunks)
  • bridging/android/jni/graphics/NativeRendererInterface.h (2 hunks)
  • bridging/android/jni/graphics/NativeSceneInterface.cpp (2 hunks)
  • bridging/android/jni/map/NativeLayerInterface.cpp (3 hunks)
  • bridging/android/jni/map/NativeLayerInterface.h (2 hunks)
  • bridging/android/jni/map/NativeMapInterface.cpp (2 hunks)
  • bridging/ios/MCLayerInterface+Private.mm (3 hunks)
  • bridging/ios/MCLayerInterface.h (2 hunks)
  • bridging/ios/MCMapInterface+Private.mm (2 hunks)
  • bridging/ios/MCMapInterface.h (2 hunks)
  • bridging/ios/MCRenderPassInterface+Private.mm (3 hunks)
  • bridging/ios/MCRenderPassInterface.h (2 hunks)
  • bridging/ios/MCRenderTargetInterface+Private.h (1 hunks)
  • bridging/ios/MCRenderTargetInterface+Private.mm (1 hunks)
  • bridging/ios/MCRenderTargetInterface.h (1 hunks)
  • bridging/ios/MCRendererInterface+Private.mm (3 hunks)
  • bridging/ios/MCRendererInterface.h (2 hunks)
  • bridging/ios/MCSceneInterface+Private.mm (2 hunks)
  • bridging/ios/MCSceneInterface.h (2 hunks)
  • djinni/graphics/core.djinni (4 hunks)
  • djinni/map/core.djinni (3 hunks)
  • djinni/yaml/render_target_interface.yaml (1 hunks)
  • ios/maps/Layers/AsyncLayer.swift (1 hunks)
  • ios/maps/Layers/Layer.swift (2 hunks)
  • ios/maps/Layers/RasterLayer.swift (2 hunks)
  • ios/maps/Layers/VectorLayer.swift (1 hunks)
  • ios/maps/MCMapView.swift (3 hunks)
  • ios/maps/RenderTargetTexture.swift (1 hunks)
  • ios/maps/SwiftUI/MapView.swift (1 hunks)
  • shared/public/LayerInterface.h (2 hunks)
  • shared/public/MapInterface.h (2 hunks)
  • shared/public/RenderPass.h (2 hunks)
  • shared/public/RenderPassInterface.h (2 hunks)
  • shared/public/RenderTargetInterface.h (1 hunks)
  • shared/public/RendererInterface.h (2 hunks)
  • shared/public/SceneInterface.h (2 hunks)
  • shared/public/SimpleLayerInterface.h (2 hunks)
  • shared/src/graphics/RenderPass.cpp (2 hunks)
  • shared/src/graphics/Renderer.cpp (3 hunks)
  • shared/src/graphics/Renderer.h (1 hunks)
  • shared/src/graphics/Scene.cpp (1 hunks)
  • shared/src/graphics/Scene.h (1 hunks)
  • shared/src/map/MapScene.cpp (1 hunks)
  • shared/src/map/MapScene.h (2 hunks)
  • shared/src/map/camera/MapCamera3d.cpp (33 hunks)
  • shared/src/map/layers/effect/SphereEffectLayer.cpp (1 hunks)
  • shared/src/map/layers/icon/IconLayer.cpp (1 hunks)
  • shared/src/map/layers/icosahedron/IcosahedronLayer.cpp (1 hunks)
  • shared/src/map/layers/line/LineLayer.cpp (1 hunks)
  • shared/src/map/layers/polygon/PolygonLayer.cpp (1 hunks)
  • shared/src/map/layers/text/TextLayer.cpp (1 hunks)
  • shared/src/map/layers/tiled/raster/Tiled2dMapRasterLayer.cpp (2 hunks)
  • shared/src/map/layers/tiled/vector/Tiled2dMapVectorLayer.cpp (1 hunks)
  • shared/src/map/layers/tiled/vector/sublayers/background/Tiled2dMapVectorBackgroundSubLayer.cpp (1 hunks)
✅ Files skipped from review due to trivial changes (6)
  • bridging/ios/MCRenderTargetInterface.h
  • shared/public/RenderTargetInterface.h
  • djinni/yaml/render_target_interface.yaml
  • bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/RenderTargetInterface.kt
  • shared/src/map/camera/MapCamera3d.cpp
  • bridging/ios/MCRenderTargetInterface+Private.h
🧰 Additional context used
🪛 SwiftLint (0.57.0)
ios/maps/MCMapView.swift

[Warning] 172-172: where clauses are preferred over a single if inside a for

(for_where)

🪛 cppcheck (2.10-2)
bridging/android/jni/graphics/NativeRenderTargetInterface.cpp

[error] 21-21: syntax error

(syntaxError)

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: build
🔇 Additional comments (94)
bridging/android/java/io/openmobilemaps/mapscore/shared/map/layers/text/FormattedStringEntry.kt (1)

Line range hint 10-24: The comparison logic is well-structured and correct.

The implementation properly maintains total ordering by:

  1. First comparing text lexicographically
  2. Then comparing scale values if texts are equal
  3. Returning appropriate values (-1, 0, 1) for each case
bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/common/Vec2D.kt (1)

Line range hint 1-1: Review auto-generation process for this change.

This file is auto-generated from common.djinni. Consider making this change in the source template instead of the generated file to ensure it persists across regeneration.

Let's check for other auto-generated files with similar patterns:

✅ Verification successful

Auto-generated files show consistent implementation pattern

The initialization of tempResult = 0 is consistently implemented across all 11 auto-generated files in the bridging layer, indicating this is a deliberate template-level change. The implementation is correct and follows the established pattern.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other auto-generated files with similar patterns
rg -l "AUTOGENERATED FILE" | xargs rg "var tempResult = 0"

Length of output: 1287

bridging/android/java/io/openmobilemaps/mapscore/shared/map/coordinates/QuadCoord.kt (1)

18-18: 🛠️ Refactor suggestion

Update the variable initialization in the Djinni source definition.

The initialization of tempResult to 0 should be defined in the source Djinni file to ensure the change persists across regeneration and remains consistent across all generated implementations.

Let's verify the Djinni source file location and content:

bridging/android/java/io/openmobilemaps/mapscore/shared/map/coordinates/Coord.kt (1)

Line range hint 1-1: Changes should be made in the Djinni source file.

This is an auto-generated file. Any changes should be made in the source coordinate_system.djinni file instead of modifying this file directly.

Let's verify the Djinni source file:

✅ Verification successful

Auto-generated file confirmed - changes must be made in the source.

The coord record in the Djinni source file (coord.yaml) derives the ord trait, which is responsible for generating the comparison implementation. Any modifications to the comparison logic should be made through the Djinni tooling rather than editing the auto-generated Kotlin file directly.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Find and display the Djinni source file
fd -g "*.djinni" -x cat {} \; | rg -A 5 "coord"

Length of output: 17435

ios/maps/Layers/Layer.swift (1)

25-27: LGTM! Default implementation looks good.

The nil default implementation makes this an optional protocol requirement, allowing conforming types to opt-in as needed.

ios/maps/Layers/AsyncLayer.swift (1)

32-34: LGTM! Proper delegation to baseLayer.

The implementation correctly forwards the beforeAdding callback to the baseLayer, maintaining the async wrapper pattern.

ios/maps/Layers/RasterLayer.swift (1)

12-16: LGTM! Consistent implementation across initializers.

The beforeAdding callback is properly integrated across all initializers with appropriate property storage.

Also applies to: 34-36, 46-46

ios/maps/Layers/VectorLayer.swift (1)

19-23: LGTM! Proper implementation of the beforeAdding callback.

The callback is correctly integrated into the initializer and stored in the property.

bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/common/Vec3D.kt (1)

13-13: ⚠️ Potential issue

⚠️ Avoid modifying auto-generated files directly.

This file is auto-generated by Djinni from common.djinni. Instead of modifying this file directly, please update the source template in common.djinni to ensure your changes persist through regeneration.

Let's verify the source template location:

bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/common/Vec2F.kt (2)

Line range hint 1-1: Warning: Modifications to autogenerated file.

This file is marked as autogenerated from common.djinni. Any direct modifications to this file will be overwritten. Changes should be made in the source .djinni file instead.

Let's verify if the source file exists and check its content:

✅ Verification successful

Warning: Direct modifications to generated code will be lost

The file is confirmed to be generated from common.djinni where vec_2_f is defined. Any changes to initialization behavior should be made through the djinni generator configuration or implementation, not by modifying the generated Kotlin file directly.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Find and display the content of the source djinni file
fd -e djinni "common.djinni" -x cat {}

Length of output: 1401


12-12: Improved safety with explicit initialization.

Good change. Initializing tempResult to 0 explicitly improves code safety by ensuring a defined initial state. This aligns with similar improvements made across other comparable classes in the codebase.

Let's verify this pattern across other similar classes:

✅ Verification successful

Consistent initialization pattern confirmed across codebase

The explicit initialization of tempResult to 0 follows an established pattern found in all comparable classes within the graphics package, including Vec2D, Vec3D, Color, and RasterShaderStyle.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for similar comparison implementations in other classes
rg -A 3 "override fun compareTo.*\{[\n\r\s]*var tempResult = 0"

Length of output: 65


Script:

#!/bin/bash
# Search with broader patterns
echo "=== Searching for compareTo implementations ==="
rg "override fun compareTo" -A 5 bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/

echo -e "\n=== Searching for tempResult variable declarations ==="
rg "var tempResult" bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/

echo -e "\n=== Searching for similar Vec2F files ==="
fd Vec bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/

Length of output: 6831

bridging/ios/MCSceneInterface.h (2)

9-9: LGTM! Protocol declaration follows conventions.

The new MCRenderTargetInterface protocol declaration is properly positioned and follows Objective-C naming conventions.


39-39: LGTM! Method signature enhancement enables offscreen rendering.

The updated drawFrame: method signature properly accepts a nullable render target parameter, enabling flexible rendering to both screen and offscreen targets. The nullability annotation is correctly specified.

Let's verify that this interface change is consistently implemented across the codebase:

✅ Verification successful

Interface change is consistently implemented

The drawFrame: method in MCSceneInterface is properly implemented across the codebase with consistent nullable parameter handling in both the interface declaration and its private implementation.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify consistent implementation of drawFrame: method
# Expected: All implementations should accept the render target parameter

# Search for drawFrame implementations
rg -t objc -t mm "drawFrame" -A 2

Length of output: 60


Script:

#!/bin/bash
# Search in all Objective-C related files
echo "=== Searching header files ==="
rg "drawFrame:" --type-add 'objc:*.{h,m,mm}' -t objc -A 2

echo -e "\n=== Looking for method implementations ==="
ast-grep --pattern '- (void)drawFrame:' 

Length of output: 2162

bridging/android/java/io/openmobilemaps/mapscore/shared/map/coordinates/RectCoord.kt (1)

16-16: ⚠️ Potential issue

Modify the Djinni definition instead of the autogenerated file.

While initializing tempResult to 0 is a good safety improvement, this file is autogenerated from a Djinni definition file and should not be modified directly. Please make this change in the source Djinni definition file instead.

Let's locate the source Djinni definition file:

✅ Verification successful

Modify the Djinni definition in djinni/map/coordinates/coordinate_system.djinni instead of the autogenerated file.

The review comment is correct. The file RectCoord.kt is autogenerated from the Djinni definition. To properly implement this safety improvement, please modify the source definition in djinni/map/coordinates/coordinate_system.djinni instead of directly changing the generated file.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Find the Djinni definition file that generates RectCoord
fd -e djinni -x grep -l "RectCoord"

Length of output: 35


Script:

#!/bin/bash
# Look for any files containing "djinni" in their name or content
echo "=== Files with 'djinni' in name ==="
fd -i djinni

echo -e "\n=== Files mentioning Djinni configuration ==="
rg -i "djinni" -g "!*.kt" -g "!*.java" -g "!*.class" -A 2 -B 2

echo -e "\n=== Looking for autogenerated file markers ==="
rg "DO NOT MODIFY|This file is generated" "bridging/android"

Length of output: 69149

bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/common/Color.kt (1)

Line range hint 1-1: Warning: Modifications to autogenerated file.

This file is marked as autogenerated with an explicit warning not to modify it directly. Instead of modifying this file, the changes should be made in the source common.djinni file.

Let's verify the source file location:

✅ Verification successful

Warning: Modifications to autogenerated file confirmed.

The file Color.kt is indeed autogenerated from djinni/graphics/common/common.djinni. Any changes to the comparison logic should be made in the source common.djinni file where the color record is defined with deriving (eq, ord) specifications.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Find the djinni source file
fd -g "*.djinni" -x grep -l "Color"

Length of output: 35


Script:

#!/bin/bash
# First, let's find any .djinni files regardless of content
echo "=== Searching for .djinni files ==="
fd -e djinni

echo "=== Searching for build files that might reference the source ==="
fd -g "*build*" -g "*cmake*" -g "*make*" -t f -x grep -l "djinni"

echo "=== Looking for Color class definition or references ==="
rg -l "class.*Color" --type kotlin --type java

Length of output: 2100


Script:

#!/bin/bash
# Check the content of common.djinni for Color definition
echo "=== Content of common.djinni ==="
cat djinni/graphics/common/common.djinni

Length of output: 1478

bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/common/Quad3dD.kt (1)

Line range hint 1-1: Avoid modifying autogenerated files.

This file is marked as autogenerated from common.djinni. Modifying autogenerated files directly is discouraged as changes will be lost when the file is regenerated. Instead, consider making the necessary changes in the source Djinni definition file.

Let's verify if there are other autogenerated files with similar modifications:

Also applies to: 14-14

bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/shader/RasterShaderStyle.kt (1)

28-28: LGTM, but verify the Djinni template.

The initialization of tempResult to 0 is correct and prevents potential issues with uninitialized variables. However, since this is an autogenerated file, ensure this change is also reflected in the Djinni template to maintain consistency across regenerations.

Run this script to locate the Djinni template file:

✅ Verification successful

Verified: The initialization is correct and matches the Djinni template

The initialization of tempResult = 0 is a correct implementation generated from the Djinni template where RasterShaderStyle is defined with the ord trait for comparison operations. No template changes are needed as this is the expected behavior.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find the Djinni template file that generates this code
# Expected: Find files named shader.djinni or similar

fd -e djinni . | xargs rg -l "RasterShaderStyle"

Length of output: 48


Script:

#!/bin/bash
# First, let's find all files that might be Djinni-related
echo "=== Looking for Djinni-related files ==="
fd djinni -t f

echo -e "\n=== Looking for files containing shader-related content ==="
rg -l -i "shader.*style" 

echo -e "\n=== Looking specifically in bridging directory ==="
fd . bridging -t f | rg -l "shader|djinni"

Length of output: 4911


Script:

#!/bin/bash
echo "=== Content of shader.djinni ==="
cat djinni/graphics/shader/shader.djinni

echo -e "\n=== Content of raster_shader_style.yaml ==="
cat djinni/yaml/raster_shader_style.yaml

Length of output: 6622

bridging/android/jni/graphics/NativeSceneInterface.cpp (2)

115-121: LGTM! Proper handling of optional render targets.

The implementation correctly:

  • Accepts an optional render target parameter
  • Uses appropriate type conversion for the optional parameter
  • Maintains existing error handling patterns

Let's verify the error handling implementation in the C++ side:

✅ Verification successful

Error handling for optional render target is properly implemented

The implementation has comprehensive error handling:

  • Proper exception translation in JNI/Objective-C bridges
  • Null-safety through consistent nullable annotations
  • Runtime checks for required components
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check error handling in SceneInterface for render target

# Look for error handling patterns in SceneInterface implementation
rg -A 5 'drawFrame.*RenderTarget' 

Length of output: 11449


Line range hint 1-1: Verify auto-generation of JNI bindings.

Since this is an auto-generated file, ensure that the Djinni configuration (core.djinni) has been properly updated to include the new render target interface and its dependencies.

Also applies to: 5-8

✅ Verification successful

Auto-generated file correctly reflects Djinni configuration

The auto-generated JNI bindings are properly configured. The render_target_interface is defined in djinni/graphics/core.djinni and the scene_interface.draw_frame() method correctly accepts an optional render target parameter.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify Djinni configuration includes render target interface

# Check if core.djinni exists and contains render target interface
fd -t f 'core.djinni' -x grep -l 'RenderTargetInterface'

# Look for other Djinni interface definitions that might need updates
fd -t f 'core.djinni' -x grep -l 'SceneInterface'

Length of output: 102


Script:

#!/bin/bash
# Find Djinni interface definition files
echo "=== Looking for Djinni interface files ==="
fd -e djinni -e idl

echo "=== Searching for interface definitions in all files ==="
rg -l "interface\s+(?:Scene|RenderTarget)" --type-add 'idl:*.{djinni,idl}' -t idl

echo "=== Checking common interface directories ==="
ls -R interface/ idl/ 2>/dev/null || true

Length of output: 1569


Script:

#!/bin/bash
echo "=== Checking graphics core.djinni ==="
cat djinni/graphics/core.djinni

echo -e "\n=== Checking graphics common.djinni ==="
cat djinni/graphics/common/common.djinni

Length of output: 5006

bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/common/Quad2dD.kt (2)

Line range hint 1-1: ⚠️ Manual modification of autogenerated file detected.

This file is marked as AUTOGENERATED with an explicit "DO NOT MODIFY!" warning. Changes should be made to the source template (common.djinni) instead of directly modifying this file.

Let's verify the source of this file:

✅ Verification successful

⚠️ Confirmed: This is an autogenerated file that should not be modified directly

The file is part of the djinni code generation system, with its source definition in djinni/yaml/quad_2d_d.yaml. Any changes should be made to the source YAML definition instead of modifying the generated Kotlin file directly. Manual modifications to generated files:

  • Will be overwritten during the next code generation
  • Risk introducing platform-specific inconsistencies
  • Break the cross-platform synchronization
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for the Djinni source file
fd -g "*.djinni" -x cat {} \; | grep -A 5 "Quad2dD"

Length of output: 54


Script:

#!/bin/bash
# First, find all djinni files
echo "=== Searching for djinni files ==="
fd -e djinni

# Look for build configuration files that might reference code generation
echo -e "\n=== Searching for build files mentioning code generation ==="
fd -e gradle -e yaml -e xml | xargs rg -l "djinni|codegen|generate"

# Search for any files mentioning Quad2dD to find its origin
echo -e "\n=== Searching for files mentioning Quad2dD ==="
rg "Quad2dD" -l

Length of output: 12928


14-14: Verify if this change is intentional or a side effect of code generation.

While initializing tempResult to 0 is technically correct (as it matches the final fallback return value), this change to an autogenerated file raises concerns:

  1. The modification might be lost when the file is regenerated
  2. Similar autogenerated files might need consistent updates
  3. The change seems unrelated to the PR's objective of implementing offscreen render targets

Let's check for similar patterns in other autogenerated files:

✅ Verification successful

The initialization of tempResult is a correct part of the autogenerated code pattern

The change is consistent with Djinni's code generation pattern, as evidenced by:

  • The same initialization pattern in Vec2D.kt and other comparable types
  • The source definition in common.djinni showing Quad2dD derives from ord
  • The presence of "AUTOGENERATED FILE" markers confirming this is generated code

The initialization is part of the standard implementation for Djinni-generated Comparable types and not a manual modification.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Look for similar comparison implementations in other autogenerated files
rg -l "AUTOGENERATED FILE" | xargs rg "override fun compareTo.*tempResult"

Length of output: 76


Script:

#!/bin/bash
# Check if the file contains autogenerated markers
cat bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/common/Quad2dD.kt

# Find similar class implementations
ast-grep --pattern 'class $_ {
  $$$
  override fun compareTo($_) {
    $$$
    tempResult
    $$$
  }
}'

Length of output: 1140


Script:

#!/bin/bash
# Find the source djinni file
fd common.djinni

# Find other files generated by Djinni
rg -l "generated by Djinni from common.djinni"

# Look for similar Vec2D implementation as it's used in Quad2dD
fd Vec2D.kt

Length of output: 5954


Script:

#!/bin/bash
# Check the source djinni file
cat djinni/graphics/common/common.djinni

# Check Vec2D implementation
cat bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/common/Vec2D.kt

Length of output: 2335

bridging/ios/MCRenderTargetInterface+Private.mm (1)

1-73: Implementation of MCRenderTargetInterfaceCppProxy is correct.

The code correctly implements the Objective-C proxy class MCRenderTargetInterfaceCppProxy for bridging between Objective-C and C++ using Djinni. The methods toCpp and fromCppOpt are properly defined to handle the conversions between Objective-C and C++ types, ensuring seamless integration.

bridging/ios/MCRenderPassInterface.h (2)

9-9: Forward declaration of MCRenderTargetInterface is appropriate.

Adding @protocol MCRenderTargetInterface; allows the MCRenderPassInterface to reference the MCRenderTargetInterface without requiring an import, which helps prevent circular dependencies.


20-21: New method getRenderTargetInterface added correctly.

The method - (nullable id<MCRenderTargetInterface>)getRenderTargetInterface; appropriately extends the MCRenderPassInterface protocol by providing access to an optional render target interface, aligning with the enhancements introduced in the pull request.

bridging/ios/MCRendererInterface.h (2)

8-8: Forward declaration of MCRenderTargetInterface is appropriate.

Including @protocol MCRenderTargetInterface; ensures that the compiler recognizes the protocol used in the interface, promoting modularity and reducing compile-time dependencies.


20-21: Updated drawFrame method signature correctly.

The addition of the target parameter to the drawFrame method:

- (void)drawFrame:(nullable id<MCRenderingContextInterface>)renderingContext
           camera:(nullable id<MCCameraInterface>)camera
           target:(nullable id<MCRenderTargetInterface>)target;

enhances the renderer's flexibility by allowing rendering to an optional render target. This modification is consistent with the aim to support offscreen rendering and advanced rendering techniques.

shared/public/RenderPassInterface.h (2)

13-13: LGTM! Forward declaration of RenderTargetInterface.

The forward declaration is correctly placed with other interface declarations.


26-26: LGTM! New method for retrieving optional render target.

The method signature correctly uses nullable shared_ptr, which is appropriate for optional render targets. This addition aligns well with the PR's objective of supporting offscreen rendering.

shared/public/RendererInterface.h (2)

7-7: LGTM! Added necessary includes and forward declarations.

The changes properly set up the requirements for using RenderTargetInterface.

Also applies to: 12-12


24-24: LGTM! Enhanced drawFrame with optional render target.

The method signature correctly uses nullable shared_ptr for the target parameter while maintaining the thread safety requirement through the comment.

bridging/ios/MCLayerInterface.h (2)

10-10: LGTM! Added MCRenderTargetInterface import.

The import is correctly placed with other interface imports.


53-53: LGTM! Added method to set primary render target.

The method signature correctly uses nullable annotation for the optional target parameter.

shared/public/LayerInterface.h (2)

11-11: LGTM! Added RenderTargetInterface include.

The include is correctly placed with other interface includes.


58-59: LGTM! Added method to set primary render target.

The method signature correctly uses nullable shared_ptr for the optional target parameter. This addition completes the implementation of offscreen rendering capabilities across all relevant interfaces.

shared/public/SceneInterface.h (1)

9-9: Verify autogenerated file source.

This file is marked as autogenerated from core.djinni. The changes look consistent, but ensure that:

  1. The corresponding Djinni IDL file has been updated
  2. The file was regenerated using the correct Djinni generator version

Also applies to: 12-12, 41-41

shared/src/graphics/RenderPass.cpp (3)

13-15: LGTM! Constructor changes are well-structured.

The addition of the renderTargetInterface parameter is consistent with the class's responsibility to manage render targets.


18-21: LGTM! Consistent parameter handling in both constructors.

The second constructor maintains consistency with the first one in terms of parameter ordering and initialization.


31-33: LGTM! Simple and effective getter implementation.

The getRenderTargetInterface method follows the established pattern of other getters in the class.

shared/src/graphics/Scene.cpp (1)

43-46: LGTM! Implementation correctly handles the new render target parameter.

The changes properly integrate the new render target parameter into the rendering pipeline, maintaining consistency with the interface changes.

shared/src/graphics/Scene.h (1)

38-38: LGTM! Interface properly documents the nullable parameter.

The header file correctly reflects the implementation changes and includes appropriate nullable annotation for the target parameter.

bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/RendererInterface.kt (2)

16-16: LGTM! Interface properly declares the nullable parameter.

The abstract method signature correctly specifies the optional render target parameter.


48-52: LGTM! CppProxy implementation maintains consistency with native layer.

The implementation and native method declarations properly handle the new parameter, maintaining the bridge between Kotlin and C++.

bridging/android/jni/graphics/NativeRendererInterface.h (2)

38-38: LGTM! JavaProxy interface properly handles parameter nullability.

The method signature correctly specifies the nullability constraints for all parameters.


48-48: Verify the JNI method signature string.

Please verify that the JNI signature string for the drawFrame method is correct for handling nullable parameters in JNI.

✅ Verification successful

JNI method signature is correctly formatted

The current JNI signature is properly formatted and doesn't require modifications. In JNI, reference parameters are nullable by default, and nullability annotations don't affect the signature format. The C++ code already handles nullability constraints through explicit annotations.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the JNI signature for drawFrame method
# Expected: The method signature in the .class file should match our JNI signature

# Find the RendererInterface.class file
fd -e class RendererInterface$ -x javap -s {} | grep -A 1 "drawFrame("

Length of output: 76


Script:

#!/bin/bash
# Find Java interface/implementation files
fd -e java "RendererInterface" -x cat {}

# Backup: Search for drawFrame method signature
ast-grep --pattern 'void drawFrame($$$)'

Length of output: 695

djinni/graphics/core.djinni (1)

89-89: Verify draw_frame implementation across platforms.

The addition of the optional render target parameter needs to be properly handled in all platform-specific implementations.

bridging/android/jni/graphics/NativeRenderPassInterface.h (1)

39-39: Verify JNI reference handling for RenderTargetInterface.

Ensure proper handling of JNI local references when returning RenderTargetInterface objects.

Also applies to: 51-51

✅ Verification successful

JNI reference handling is properly managed by Djinni framework.

The codebase consistently uses Djinni's JNI helper library which automatically handles JNI local references and provides type-safe conversions for shared_ptr objects, including proper null handling for RenderTargetInterface.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for RenderTargetInterface reference handling in JNI code
echo "Searching for RenderTargetInterface reference handling:"
rg -p "RenderTargetInterface.*NewLocalRef|RenderTargetInterface.*DeleteLocalRef" --type cpp

Length of output: 214


Script:

#!/bin/bash
# Find JNI implementation files related to RenderTargetInterface
echo "Searching for JNI implementation files:"
fd -e cpp -e h . bridging/android/jni/graphics

echo -e "\nSearching for getRenderTargetInterface implementation:"
ast-grep --pattern 'JNIEXPORT jobject JNICALL Java_$$$getRenderTargetInterface'

echo -e "\nSearching for shared_ptr conversion patterns:"
rg -p "djinni::get" bridging/android/jni/graphics -A 3

Length of output: 47598

bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/RenderPassInterface.kt (1)

56-61: Verify native resource cleanup.

Ensure that native resources associated with RenderTargetInterface are properly cleaned up when the object is destroyed.

✅ Verification successful

Resource cleanup properly implemented

The RenderTargetInterface implementation follows proper resource management patterns:

  • C++ RAII with virtual destructor
  • Automatic cleanup via shared_ptr
  • JNI bridge with proper native resource handling
  • Kotlin implementation with destroyed state validation
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for native cleanup code
echo "Searching for RenderTargetInterface cleanup:"
rg -p "nativeDestroy.*RenderTargetInterface" --type cpp --type kotlin

Length of output: 170


Script:

#!/bin/bash
# Search for RenderTargetInterface usage and cleanup patterns
echo "Searching for RenderTargetInterface patterns:"
rg -p "RenderTargetInterface" -A 5 -B 5

echo -e "\nSearching for destroy patterns in graphics:"
fd . -e kt -e cpp | grep -i "graphics" | xargs rg "destroy" -A 3 -B 3

echo -e "\nSearching for cleanup patterns:"
rg "cleanup|dispose|release" -g "**/*RenderTarget*" -g "**/*RenderPass*"

Length of output: 74702

bridging/ios/MCMapInterface.h (2)

9-9: LGTM!

The import for MCRenderTargetInterface.h is correctly placed in alphabetical order.


88-91: LGTM!

The new methods are well-defined with:

  • Clear method names following Objective-C conventions
  • Proper use of nullable annotation for optional parameters
  • Consistent placement within the interface
shared/src/map/MapScene.h (2)

76-78: LGTM!

The new virtual methods are well-defined with:

  • Clear method names
  • Proper override specifiers
  • Consistent declaration style

110-110: LGTM!

The needsCompute member variable is:

  • Correctly placed in private section
  • Properly initialized to false
  • Follows class member naming conventions
shared/src/graphics/Renderer.cpp (1)

90-92: Verify the impact of conditional renderQueue clearing.

The change to only clear the renderQueue when target is null represents a significant behavioral change:

  • Previous behavior: Queue was always cleared after each frame
  • New behavior: Queue persists when rendering to a target

Please verify that:

  1. Subsequent frames handle the persistent queue correctly
  2. No memory leaks occur from retained render passes
  3. Frame-to-frame state remains consistent
✅ Verification successful

The conditional queue clearing is safe and intentional

The implementation is correct because:

  • RenderQueue uses shared_ptr for automatic memory management
  • Passes are filtered by target during rendering
  • Queue clearing on null target provides a natural frame boundary
  • Non-matching passes are preserved for subsequent target renders

The behavioral change from always clearing to conditional clearing is intentional and properly handles multi-target rendering while maintaining frame-to-frame consistency.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other renderQueue modifications to understand state management
rg -A 5 "renderQueue\." --type cpp

Length of output: 404


Script:

#!/bin/bash
# Find RenderQueue class definition and usage
ast-grep --pattern 'class RenderQueue'

# Look for queue additions and the render loop
rg "renderQueue" --type cpp -B 2 -A 5

# Find render/frame-related methods in Renderer
ast-grep --pattern 'class Renderer {
  $$$
  render($$$) {
    $$$
  }
  $$$
}'

Length of output: 2289

bridging/ios/MCSceneInterface+Private.mm (1)

111-114: LGTM! Bridging implementation is correct.

The implementation properly handles the nullable render target parameter and correctly uses the djinni Optional wrapper for type conversion.

bridging/android/jni/map/NativeLayerInterface.h (1)

53-53: LGTM! JNI bridging implementation is correct.

The implementation properly handles the nullable render target parameter and correctly sets up the JNI method binding.

Also applies to: 77-77

bridging/android/jni/graphics/NativeRenderPassInterface.cpp (1)

46-53: LGTM! JNI bridging implementation for render target retrieval.

The implementation correctly handles JNI bridging with proper scope and exception management.

djinni/map/core.djinni (1)

135-136: Verify render target lifecycle management.

The set_primary_render_target method allows changing render targets dynamically. Ensure proper cleanup of previous render targets to prevent memory leaks.

✅ Verification successful

Memory management is properly handled through smart pointers

The render target lifecycle is safely managed through std::shared_ptr and RAII principles. The set_primary_render_target method is designed to automatically clean up previous render targets when they are no longer referenced, preventing memory leaks.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for render target cleanup patterns
rg -A 5 "set_primary_render_target|cleanup|dispose" --type cpp

Length of output: 949


Script:

#!/bin/bash
# Find render target interface implementations
fd -e cpp -e h render_target

# Search for render target related code
rg -A 10 "class.*RenderTarget|set_primary_render_target" --type cpp --type hpp

Length of output: 139


Script:

#!/bin/bash
# Find render target related files with correct extensions
fd -e cpp -e c -e h -e hpp render

# Search for render target patterns in source files
rg -A 10 "class.*[Rr]ender.*[Tt]arget|set_primary_render_target" --type cpp --type c --type h

# Search for memory management patterns around render targets
rg -A 5 "delete.*[Rr]ender.*[Tt]arget|release.*[Rr]ender.*[Tt]arget|clean.*[Rr]ender.*[Tt]arget" --type cpp --type c --type h

Length of output: 10332

shared/src/map/layers/text/TextLayer.cpp (1)

193-193: Verify renderTarget initialization.

The renderTarget variable is used in the RenderPass constructor but its initialization is not visible in the provided code. Ensure it's properly initialized before use.

bridging/ios/MCLayerInterface+Private.mm (1)

17-17: LGTM!

The changes follow the established patterns for bridging between Objective-C and C++:

  • Import statement is correctly placed.
  • Methods properly handle nullable parameters and exceptions.
  • Type conversions are handled correctly.

Also applies to: 150-154, 272-277

bridging/ios/MCMapInterface+Private.mm (1)

19-19: LGTM!

The changes follow the established patterns for bridging between Objective-C and C++:

  • Import statement is correctly placed.
  • Methods properly handle nullable parameters, return types, and exceptions.
  • Type conversions are handled correctly.

Also applies to: 236-247

shared/src/map/layers/line/LineLayer.cpp (1)

202-202: Verify the scope and initialization of renderTarget.

The renderTarget variable is used in the RenderPass constructor but its declaration and initialization are not visible in the provided code.

Run the following script to verify the variable's scope and initialization:

✅ Verification successful

renderTarget is properly scoped and initialized

The variable is a protected member inherited from SimpleLayerInterface and is initialized through the setPrimaryRenderTarget method. This is a standard pattern used consistently across all layer implementations.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the scope and initialization of renderTarget variable.

# Test: Search for renderTarget declaration and initialization.
rg -A 5 $'renderTarget\s*=' LineLayer

# Test: Search for renderTarget parameter in the class declaration.
ast-grep --pattern $'class LineLayer {
  $$$
  renderTarget
  $$$
}'

Length of output: 158


Script:

#!/bin/bash
# Find the correct file path
fd LineLayer.cpp

# Search for renderTarget in the file
rg -B 5 -A 5 "renderTarget" --type cpp

# Search for the function containing this RenderPass construction
ast-grep --pattern 'renderTarget = $_'

# Search for class definition and member variables
ast-grep --pattern 'class LineLayer {
  $$$
}'

Length of output: 18033

bridging/android/java/io/openmobilemaps/mapscore/shared/map/MapInterface.kt (1)

244-254: Implementation looks good!

The CppProxy implementation follows the established patterns with proper null checks and exception handling.

ios/maps/SwiftUI/MapView.swift (1)

274-276: Implementation looks good!

The beforeAdding closure is properly handled with null safety using optional chaining.

ios/maps/MCMapView.swift (1)

207-247: Implementation follows Metal best practices!

The changes properly handle:

  • Frame initialization
  • Conditional compute pass creation
  • Offscreen render target setup
bridging/android/jni/map/NativeLayerInterface.cpp (1)

155-162: JNI implementation looks good!

The implementation follows djinni patterns with proper JNI environment setup and exception handling.

Also applies to: 312-318

bridging/android/jni/map/NativeMapInterface.cpp (3)

16-16: LGTM!

The include is properly placed and required for the new offscreen rendering functionality.


260-267: LGTM!

The method is properly implemented with:

  • Correct JNI naming conventions
  • Proper exception handling
  • Appropriate boolean type conversion

269-275: LGTM!

The method is properly implemented with:

  • Correct JNI naming conventions
  • Proper exception handling
  • Appropriate render target type conversion
shared/src/map/MapScene.cpp (5)

381-382: LGTM!

The variable is properly initialized to false before checking compute passes.


390-390: LGTM!

The flag is correctly set when a compute pass is added to the queue.


396-398: LGTM!

Simple getter method that provides access to the compute state.


405-406: LGTM!

Explicitly passing nullptr maintains backward compatibility with the new render target parameter.


408-409: LGTM!

The method properly handles offscreen rendering by forwarding the non-null render target.

shared/src/map/layers/icon/IconLayer.cpp (1)

317-317: LGTM!

The RenderPass constructor call is properly updated to include the render target parameter.

shared/src/map/layers/tiled/raster/Tiled2dMapRasterLayer.cpp (2)

450-450: LGTM!

The RenderPass constructor call is properly updated to include the render target parameter.


464-464: LGTM!

The RenderPass constructor call is properly updated to include the render target parameter.

shared/src/map/layers/polygon/PolygonLayer.cpp (1)

256-256: LGTM! Consistent render target integration.

The change correctly adds the render target parameter to the RenderPass constructor call, maintaining consistency with the broader offscreen rendering feature implementation.

bridging/android/jni/graphics/NativeRenderTargetInterface.cpp (3)

8-14: LGTM! Well-structured JNI implementation.

The implementation follows JNI best practices with proper initialization and cleanup:

  • Correct Java class path for JNI interface
  • Appropriate use of default destructors
  • Clean proxy handle implementation

17-22: LGTM! Proper resource cleanup implementation.

The native destroy method correctly handles the cleanup of C++ resources with appropriate error translation. The cppcheck syntax error can be safely ignored as the JNI_TRANSLATE_EXCEPTIONS_RETURN macro is a valid Djinni construct, and the empty second argument is intentional for void return types.

🧰 Tools
🪛 cppcheck (2.10-2)

[error] 21-21: syntax error

(syntaxError)


1-5: Note: This is an auto-generated file.

Since this file is auto-generated from Djinni interface definitions, any necessary changes should be made to the source .djinni file instead of modifying this file directly.

Let's verify the source Djinni file:

✅ Verification successful

Auto-generated file verification confirmed

This file is correctly auto-generated from the render_target_interface.yaml definition. Any modifications should be made to the YAML source file instead of this JNI implementation. The interface is an integral part of the rendering system and is referenced by multiple components.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Find and display the Djinni interface definition
fd -e djinni -x cat {} | grep -A 5 "RenderTargetInterface"

Length of output: 60


Script:

#!/bin/bash
# First, let's find all .djinni files
echo "=== All .djinni files ==="
fd -e djinni

# Then search for render-related terms in these files
echo -e "\n=== Searching for render terms ==="
fd -e djinni -x rg -i "render|target" {}

Length of output: 5970

bridging/android/java/io/openmobilemaps/mapscore/shared/map/LayerInterface.kt (1)

46-47: LGTM! Clean implementation of setPrimaryRenderTarget.

The implementation properly handles nullable render targets and follows the established patterns in the codebase.

Also applies to: 165-169

shared/src/map/layers/tiled/vector/Tiled2dMapVectorLayer.cpp (1)

697-697: Verify renderTarget initialization.

The changes consistently pass the renderTarget parameter to RenderPass constructors, which is good. However, please ensure that the renderTarget variable is properly initialized before use.

Run this script to locate where renderTarget is defined:

Also applies to: 705-705, 709-709, 717-717

shared/public/RenderPass.h (1)

19-21: LGTM! Well-designed RenderPass interface changes.

The changes maintain backward compatibility through default nullptr parameters and follow good C++ practices:

  • Virtual method for polymorphic behavior
  • Clear member variable ownership with shared_ptr
  • Consistent interface naming

Also applies to: 27-28, 41-41

bridging/android/jni/graphics/NativeRenderTargetInterface.h (1)

1-44: Auto-generated file, no manual changes needed.

This is an auto-generated JNI binding file by Djinni. The implementation follows standard patterns for JNI interfaces.

bridging/ios/MCRendererInterface+Private.mm (3)

13-13: LGTM: Import statement properly added.

The new import for RenderTargetInterface is correctly placed with other related imports.


52-57: LGTM: Method signature properly updated.

The drawFrame method signature is correctly updated to include the nullable RenderTarget parameter, maintaining consistency with the Android implementation.


90-96: LGTM: Proxy implementation properly handles nullable target.

The ObjcProxy implementation correctly handles the nullable RenderTarget parameter and maintains proper memory management.

bridging/android/java/io/openmobilemaps/mapscore/shared/graphics/SceneInterface.kt (2)

35-35: LGTM: Abstract method signature properly updated.

The drawFrame method signature is correctly updated to include the nullable RenderTarget parameter.


106-110: LGTM: CppProxy implementation properly handles nullable target.

The implementation correctly:

  • Maintains the destroyed state check
  • Passes the target parameter to the native layer
  • Updates the native method signature
bridging/ios/MCRenderPassInterface+Private.mm (3)

14-14: LGTM: Import statement properly added.

The new import for RenderTargetInterface is correctly placed with other related imports.


59-64: LGTM: New method properly implemented.

The getRenderTargetInterface method is correctly implemented with:

  • Proper exception handling
  • Correct type conversion using djinni
  • Proper nullability annotations

109-115: LGTM: Proxy implementation properly handles nullable return type.

The ObjcProxy implementation correctly handles the nullable return type and maintains proper memory management.

bridging/android/jni/graphics/NativeRendererInterface.cpp (3)

5-9: LGTM: Required headers properly included.

The new headers for Marshal.hpp and NativeRenderTargetInterface.h are correctly added.


38-46: LGTM: JavaProxy implementation properly updated.

The drawFrame implementation correctly:

  • Updates method signature with nullable target parameter
  • Maintains proper JNI environment handling
  • Uses correct type conversion for the new parameter

81-88: LGTM: Native method implementation properly updated.

The native drawFrame implementation correctly:

  • Updates method signature with target parameter
  • Maintains proper exception handling
  • Uses correct type conversion for all parameters

Comment on lines +69 to +77
for _ in 0 ..< RenderingContext.bufferCount {
texDescriptor.pixelFormat = MetalContext.current.colorPixelFormat
let texture = MetalContext.current.device.makeTexture(descriptor: texDescriptor)!

texDescriptor.pixelFormat = .stencil8
let stencilTexture = MetalContext.current.device.makeTexture(descriptor: texDescriptor)!

textures.append((color: texture, stencilDepth: stencilTexture))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Handle texture creation failures gracefully.

The forced unwrapping of texture creation (makeTexture(descriptor:)!) could crash if the device runs out of memory. Consider handling these cases gracefully.

 for _ in 0 ..< RenderingContext.bufferCount {
     texDescriptor.pixelFormat = MetalContext.current.colorPixelFormat
-    let texture = MetalContext.current.device.makeTexture(descriptor: texDescriptor)!
+    guard let texture = MetalContext.current.device.makeTexture(descriptor: texDescriptor) else {
+        NSLog("Error: Failed to create color texture for RenderTarget: \(name)")
+        return
+    }
 
     texDescriptor.pixelFormat = .stencil8
-    let stencilTexture = MetalContext.current.device.makeTexture(descriptor: texDescriptor)!
+    guard let stencilTexture = MetalContext.current.device.makeTexture(descriptor: texDescriptor) else {
+        NSLog("Error: Failed to create stencil texture for RenderTarget: \(name)")
+        return
+    }

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +32 to +52
public func prepareOffscreenEncoder(_ commandBuffer: MTLCommandBuffer?, size: MCVec2I, context: RenderingContext) -> MTLRenderCommandEncoder? {

setSize(size)

let renderPassDescriptor = MTLRenderPassDescriptor()
renderPassDescriptor.colorAttachments[0]?.loadAction = .clear
renderPassDescriptor.colorAttachments[0]?.clearColor = .init(red: 0, green: 0, blue: 0.0, alpha: 0.0)
renderPassDescriptor.colorAttachments[0]?.storeAction = .store
renderPassDescriptor.stencilAttachment.storeAction = .dontCare

renderPassDescriptor
.colorAttachments[0].texture = textures[context.currentBufferIndex].color
renderPassDescriptor.stencilAttachment.texture = textures[context.currentBufferIndex].stencilDepth

if let renderEncoder = commandBuffer?.makeRenderCommandEncoder(descriptor: renderPassDescriptor) {
renderEncoder.label = "Offscreen Encoder (\(name))"
return renderEncoder
}

return nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add bounds checking and improve error handling.

The array access at textures[context.currentBufferIndex] could cause a crash if textures array is empty. Consider adding proper error handling.

 public func prepareOffscreenEncoder(_ commandBuffer: MTLCommandBuffer?, size: MCVec2I, context: RenderingContext) -> MTLRenderCommandEncoder? {
+    guard !textures.isEmpty, context.currentBufferIndex < textures.count else {
+        NSLog("Error: Textures not properly initialized for RenderTarget: \(name)")
+        return nil
+    }
 
     setSize(size)
📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public func prepareOffscreenEncoder(_ commandBuffer: MTLCommandBuffer?, size: MCVec2I, context: RenderingContext) -> MTLRenderCommandEncoder? {
setSize(size)
let renderPassDescriptor = MTLRenderPassDescriptor()
renderPassDescriptor.colorAttachments[0]?.loadAction = .clear
renderPassDescriptor.colorAttachments[0]?.clearColor = .init(red: 0, green: 0, blue: 0.0, alpha: 0.0)
renderPassDescriptor.colorAttachments[0]?.storeAction = .store
renderPassDescriptor.stencilAttachment.storeAction = .dontCare
renderPassDescriptor
.colorAttachments[0].texture = textures[context.currentBufferIndex].color
renderPassDescriptor.stencilAttachment.texture = textures[context.currentBufferIndex].stencilDepth
if let renderEncoder = commandBuffer?.makeRenderCommandEncoder(descriptor: renderPassDescriptor) {
renderEncoder.label = "Offscreen Encoder (\(name))"
return renderEncoder
}
return nil
}
public func prepareOffscreenEncoder(_ commandBuffer: MTLCommandBuffer?, size: MCVec2I, context: RenderingContext) -> MTLRenderCommandEncoder? {
guard !textures.isEmpty, context.currentBufferIndex < textures.count else {
NSLog("Error: Textures not properly initialized for RenderTarget: \(name)")
return nil
}
setSize(size)
let renderPassDescriptor = MTLRenderPassDescriptor()
renderPassDescriptor.colorAttachments[0]?.loadAction = .clear
renderPassDescriptor.colorAttachments[0]?.clearColor = .init(red: 0, green: 0, blue: 0.0, alpha: 0.0)
renderPassDescriptor.colorAttachments[0]?.storeAction = .store
renderPassDescriptor.stencilAttachment.storeAction = .dontCare
renderPassDescriptor
.colorAttachments[0].texture = textures[context.currentBufferIndex].color
renderPassDescriptor.stencilAttachment.texture = textures[context.currentBufferIndex].stencilDepth
if let renderEncoder = commandBuffer?.makeRenderCommandEncoder(descriptor: renderPassDescriptor) {
renderEncoder.label = "Offscreen Encoder (\(name))"
return renderEncoder
}
return nil
}

@@ -45,7 +45,7 @@ void SphereEffectLayer::onAdded(const std::shared_ptr<MapInterface> &mapInterfac

renderObjects.push_back(std::make_shared<RenderObject>(this->quad->asGraphicsObject(), true));

auto renderPass = std::make_shared<RenderPass>(RenderPassConfig(0, false), renderObjects );
auto renderPass = std::make_shared<RenderPass>(RenderPassConfig(0, false), renderObjects, renderTarget );
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix undefined variable error.

The renderTarget variable is used in the RenderPass constructor but hasn't been declared. This will cause a compilation error.

Add the declaration before using it:

+    std::shared_ptr<RenderTargetInterface> renderTarget = nullptr;
     auto renderPass = std::make_shared<RenderPass>(RenderPassConfig(0, false), renderObjects, renderTarget );
📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
auto renderPass = std::make_shared<RenderPass>(RenderPassConfig(0, false), renderObjects, renderTarget );
std::shared_ptr<RenderTargetInterface> renderTarget = nullptr;
auto renderPass = std::make_shared<RenderPass>(RenderPassConfig(0, false), renderObjects, renderTarget );

Comment on lines +45 to +47
if (pass->getRenderTargetInterface() != target) {
continue;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add null check for render target comparison.

The direct comparison of render targets could be unsafe if either is null. Consider adding explicit null checks.

-            if (pass->getRenderTargetInterface() != target) {
+            auto passTarget = pass->getRenderTargetInterface();
+            if ((passTarget == nullptr) != (target == nullptr) || 
+                (passTarget != nullptr && target != nullptr && passTarget != target)) {
                 continue;
             }
📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (pass->getRenderTargetInterface() != target) {
continue;
}
auto passTarget = pass->getRenderTargetInterface();
if ((passTarget == nullptr) != (target == nullptr) ||
(passTarget != nullptr && target != nullptr && passTarget != target)) {
continue;
}

@@ -78,6 +79,10 @@ class MapInterface {

virtual void prepare() = 0;

virtual bool getNeedsCompute() = 0;

virtual void drawOffscreenFrame(const /*not-null*/ std::shared_ptr<::RenderTargetInterface> & target) = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Add thread safety requirement comment for consistency

The codebase shows a clear pattern where all rendering-related operations must be called on the rendering thread. The drawOffscreenFrame method should follow this pattern for consistency with drawFrame(), compute(), resume(), and pause().

+    /** Must be called on the rendering thread! */
     virtual void drawOffscreenFrame(const /*not-null*/ std::shared_ptr<::RenderTargetInterface> & target) = 0;
🔗 Analysis chain

Verify thread safety requirements.

The drawOffscreenFrame method likely needs the same thread safety requirements as drawFrame(). Consider adding a similar comment to ensure proper usage.

+    /** Must be called on the rendering thread! */
     virtual void drawOffscreenFrame(const /*not-null*/ std::shared_ptr<::RenderTargetInterface> & target) = 0;
🏁 Scripts executed

The following scripts were executed for the analysis:

Error: No shell scripts found


Script:

#!/bin/bash
# First, let's look at the content of MapInterface.h
cat shared/public/MapInterface.h

# Then, let's search for method declarations with thread safety comments
ast-grep --pattern '/** $$$thread$$$ */
    $$$'

Length of output: 4467

@@ -35,7 +35,7 @@ void IcosahedronLayer::onAdded(const std::shared_ptr<MapInterface> & mapInterfac
object = mapInterface->getGraphicsObjectFactory()->createIcosahedronObject(shader->asShaderProgramInterface());

auto renderObject = std::make_shared<RenderObject>(object->asGraphicsObject());
std::shared_ptr<RenderPass> renderPass = std::make_shared<RenderPass>(RenderPassConfig(0, false), std::vector<std::shared_ptr<::RenderObjectInterface>>{ renderObject });
std::shared_ptr<RenderPass> renderPass = std::make_shared<RenderPass>(RenderPassConfig(0, false), std::vector<std::shared_ptr<::RenderObjectInterface>>{ renderObject }, renderTarget);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix uninitialized variable usage.

The renderTarget variable is used in the RenderPass constructor but is not declared or initialized. This could lead to undefined behavior.

Apply this diff to fix the issue:

+    std::shared_ptr<::RenderTargetInterface> renderTarget = nullptr;
     std::shared_ptr<RenderPass> renderPass = std::make_shared<RenderPass>(RenderPassConfig(0, false), std::vector<std::shared_ptr<::RenderObjectInterface>>{ renderObject }, renderTarget);
📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
std::shared_ptr<RenderPass> renderPass = std::make_shared<RenderPass>(RenderPassConfig(0, false), std::vector<std::shared_ptr<::RenderObjectInterface>>{ renderObject }, renderTarget);
std::shared_ptr<::RenderTargetInterface> renderTarget = nullptr;
std::shared_ptr<RenderPass> renderPass = std::make_shared<RenderPass>(RenderPassConfig(0, false), std::vector<std::shared_ptr<::RenderObjectInterface>>{ renderObject }, renderTarget);

@@ -110,7 +110,7 @@ void Tiled2dMapVectorBackgroundSubLayer::onAdded(const std::shared_ptr<MapInterf
renderObjects.push_back(std::make_shared<RenderObject>(patternObject->getPolygonObject(), !is3d));
}

auto renderPass = std::make_shared<RenderPass>(RenderPassConfig(0, false), renderObjects );
auto renderPass = std::make_shared<RenderPass>(RenderPassConfig(0, false), renderObjects, renderTarget );
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Potential undefined variable usage detected

The renderTarget variable used in the RenderPass constructor at line 113 appears to be undefined. After thorough analysis of the file:

  • It's not declared as a class member
  • Not a parameter of the containing method (onAdded)
  • Not declared locally in the method
  • Not inherited from any visible parent class

This could lead to undefined behavior and should be properly declared and initialized before use.

🔗 Analysis chain

Verify the scope and initialization of renderTarget.

The renderTarget variable is used in the RenderPass constructor but its declaration and initialization are not visible in the provided code.

Run the following script to verify the variable's scope and initialization:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the scope and initialization of renderTarget variable.

# Test: Search for renderTarget declaration and initialization.
rg -A 5 $'renderTarget\s*=' Tiled2dMapVectorBackgroundSubLayer

# Test: Search for renderTarget parameter in the class declaration.
ast-grep --pattern $'class Tiled2dMapVectorBackgroundSubLayer {
  $$$
  renderTarget
  $$$
}'

Length of output: 233


Script:

#!/bin/bash
# Search for renderTarget in the specific file with context
rg -A 5 -B 5 "renderTarget" shared/src/map/layers/tiled/vector/sublayers/background/Tiled2dMapVectorBackgroundSubLayer.cpp

# Search for method declarations that might receive renderTarget
ast-grep --pattern 'void $_(RenderTarget $_, $$$)' shared/src/map/layers/tiled/vector/sublayers/background/Tiled2dMapVectorBackgroundSubLayer.cpp

# Search for class member declarations
ast-grep --pattern 'class Tiled2dMapVectorBackgroundSubLayer {
  $$$
  RenderTarget $_
  $$$
}'

Length of output: 847


Script:

#!/bin/bash
# Find the method containing the renderPass creation
ast-grep --pattern 'void $_($$$) {
  $$$
  auto renderPass = std::make_shared<RenderPass>($$$);
  $$$
}'

# Find class declaration with potential base classes
ast-grep --pattern 'class Tiled2dMapVectorBackgroundSubLayer : $_'

# Search for method parameters named renderTarget
rg -B 10 "void \w+\([^)]*renderTarget[^)]*\)" shared/src/map/layers/tiled/vector/sublayers/background/Tiled2dMapVectorBackgroundSubLayer.cpp

Length of output: 318


Script:

#!/bin/bash
# Check the header file
rg "RenderTarget" shared/src/map/layers/tiled/vector/sublayers/background/Tiled2dMapVectorBackgroundSubLayer.h

# Find method containing renderPass with more context
rg -B 20 "auto renderPass = std::make_shared<RenderPass>" shared/src/map/layers/tiled/vector/sublayers/background/Tiled2dMapVectorBackgroundSubLayer.cpp

# Search for RenderTarget parameter in method declarations
ast-grep --pattern '($$$RenderTarget $_ $$$)'

Length of output: 1425


Script:

#!/bin/bash
# Get the complete file content
cat shared/src/map/layers/tiled/vector/sublayers/background/Tiled2dMapVectorBackgroundSubLayer.cpp

# Search for method signatures containing render or update
rg "^[[:space:]]*(void|bool|auto)[[:space:]]+(render|update|setup|init)" shared/src/map/layers/tiled/vector/sublayers/background/Tiled2dMapVectorBackgroundSubLayer.cpp

Length of output: 11178

@stmitt stmitt changed the base branch from main to develop January 21, 2025 08:00
@maurhofer-ubique maurhofer-ubique force-pushed the feature/offscreen-render-targets branch from 05703ef to 1986705 Compare January 21, 2025 11:00
@maurhofer-ubique maurhofer-ubique force-pushed the feature/offscreen-render-targets branch from 914f9ba to 2be1d6d Compare January 21, 2025 13:37
@maurhofer-ubique maurhofer-ubique merged commit 3ebb97f into develop Jan 21, 2025
4 checks passed
@maurhofer-ubique maurhofer-ubique deleted the feature/offscreen-render-targets branch January 21, 2025 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants