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

feat: Add bounds to raster source #3672

Merged
merged 2 commits into from
Oct 30, 2024

Conversation

Elter71
Copy link
Contributor

@Elter71 Elter71 commented Oct 29, 2024

Description

Added bounds support for RasterSource . Now, you can limit the bounds within which raster tiles will render.

Checklist

  • I've read CONTRIBUTING.md
  • I updated the doc/other generated code with running yarn generate in the root folder
  • I have tested the new feature on /example app.
    • In V11 mode/ios
    • In New Architecture mode/ios
    • In V11 mode/android
    • In New Architecture mode/android
  • I added/updated a sample - if a new feature was implemented (/example)

Screenshot OR Video

Screenshot_1730212439
Simulator Screenshot - iPhone SE (3rd generation) - 2024-10-29 at 15 34 47

@Elter71 Elter71 temporarily deployed to CI with Mapbox Tokens October 29, 2024 14:44 — with GitHub Actions Inactive
@Elter71 Elter71 temporarily deployed to CI with Mapbox Tokens October 29, 2024 14:44 — with GitHub Actions Inactive
@Elter71 Elter71 temporarily deployed to CI with Mapbox Tokens October 29, 2024 14:44 — with GitHub Actions Inactive
@Elter71 Elter71 temporarily deployed to CI with Mapbox Tokens October 29, 2024 14:44 — with GitHub Actions Inactive
@Elter71 Elter71 temporarily deployed to CI with Mapbox Tokens October 29, 2024 14:44 — with GitHub Actions Inactive
@Elter71 Elter71 temporarily deployed to CI with Mapbox Tokens October 29, 2024 14:44 — with GitHub Actions Inactive
@Elter71 Elter71 temporarily deployed to CI with Mapbox Tokens October 29, 2024 14:44 — with GitHub Actions Inactive
Copy link
Contributor

@mfazekas mfazekas left a comment

Choose a reason for hiding this comment

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

@Elter71 Thanks much amazing work!

Some minor nitpicks

val readableArray: ReadableArray = value.asArray()

if (readableArray.size() == 4) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this check necessary here? We'll just silently error if the size is not 4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this, you will get an error like this:
Screenshot_1730282753

Personally, I prefer a more direct message, but I leave it up to you.

Copy link
Contributor

@mfazekas mfazekas Oct 30, 2024

Choose a reason for hiding this comment

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

Sorry, mybad, you're right I've missed the Logger.e at the end.

Nitpick: Can please you rewrite the logic so we have early return on error statements, and not, the success case

Something like:

if (value.type.name != "Array" || readableArray.size() != 4) {
   Logger.e(RNMBXRasterSourceManager.REACT_CLASS, "source bounds must be an array with left, bottom, top, and right values")
   return
}

val bboxArray = Array(4) { i -> readableArray.getDouble(i)
source.setSourceBounds(bboxArray)
...

@@ -35,10 +36,14 @@ abstract class RNMBXTileSource<T : Source?>(context: Context?) : RNMBXSource<T>(
if (attribution != null) {
builder.attribution(attribution)
}
if(bounds != null) {
val boundsArray = bounds!!.clone()
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need !! we've tested above for null already

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this, you will get an error:

Smart cast to 'Array<Double>' is impossible, because 'bounds' is a mutable property that could have been changed by this time FAILURE: Build failed with an exception.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should be able to do something like this:

bounds?.let { 
   builder.bounds(Arrays.asList(it))
}

@Elter71 Elter71 temporarily deployed to CI with Mapbox Tokens October 30, 2024 11:01 — with GitHub Actions Inactive
@Elter71 Elter71 temporarily deployed to CI with Mapbox Tokens October 30, 2024 11:01 — with GitHub Actions Inactive
@Elter71 Elter71 temporarily deployed to CI with Mapbox Tokens October 30, 2024 11:01 — with GitHub Actions Inactive
@Elter71 Elter71 temporarily deployed to CI with Mapbox Tokens October 30, 2024 11:01 — with GitHub Actions Inactive
@Elter71 Elter71 temporarily deployed to CI with Mapbox Tokens October 30, 2024 11:01 — with GitHub Actions Inactive
@Elter71 Elter71 temporarily deployed to CI with Mapbox Tokens October 30, 2024 11:01 — with GitHub Actions Inactive
@Elter71 Elter71 temporarily deployed to CI with Mapbox Tokens October 30, 2024 11:01 — with GitHub Actions Inactive
@Elter71
Copy link
Contributor Author

Elter71 commented Oct 30, 2024

@mfazekas All suggestions have been applied. Thanks for the quick response!

@mfazekas
Copy link
Contributor

@Elter71 thanks much, amazing!

@mfazekas mfazekas merged commit 63d74fc into rnmapbox:main Oct 30, 2024
11 checks passed
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