p-lr / MapView

A Fast, memory efficient Android library to display tiled maps, with support for markers, paths, and rotation.
Apache License 2.0
184 stars 38 forks source link

Added color filter option #9

Closed artudi54 closed 4 years ago

artudi54 commented 4 years ago

I have added the option to provide color filter for drawing bitmaps with canvas. It can be provided via MapViewConfiguration. By default the filter is set to null. This change does not brake the API - it's an optional parameter. If you have any issues with this change please let me know.

p-lr commented 4 years ago

So this applies a ColorFilter globally. If someone need to apply different filters for different tiles, it wouldn't work. Instead, I'm suggesting to add a new method to the TileStreamProvider interface, with a default implementation to avoid breaking exisiting implementations:

interface TileStreamProvider {
    fun getTileStream(row: Int, col: Int, zoomLvl: Int): InputStream?

    /* Must not be a blocking call  - should return immediately */
    fun getColorFilter(row: Int, col: Int, zoomLvl: Int): ColorFilter? = null
}

Then,, applying to the Paint could simply be done like that:

private fun Tile.setPaint() {
        paint = (paintPool.get() ?: Paint()).also {
            it.colorFilter = tileStreamProvider.getColorFilter(row, col, zoom)
            it.alpha = 0
        }
    }

And in recyle:

private fun Tile.recycle() {
        if (bitmap.isMutable) {
            bitmapPool.put(bitmap)
        }
        paint?.let {
            paint = null
            it.alpha = 0
            it.colorFilter = null
            paintPool.put(it)
        }
    }

What do you think?

artudi54 commented 4 years ago

If you prefer it that way I can adjust the code and update pull request. But to maintain backwards compatibility with Java, JvmDefault annotation in TileStreamProvider is also needed. I have just tested it and it requires to add to gradle this snippet in android section:

kotlinOptions {
    jvmTarget = JavaVersion.VERSION_1_8
    freeCompilerArgs = ['-Xjvm-default=compatibility']
}

Is it fine for you?

p-lr commented 4 years ago

Thanks for noticing this. After some consideration, it's a little odd to add getColorFilter inside the TileStreamProvider interface. As other options might be added in the future, I think it would be better to have a dedicated interface, like:

interface TileOptionsProvider { / Must not be a blocking call - should return immediately / fun getColorFilter(row: Int, col: Int, zoomLvl: Int): ColorFilter? = null }

A TileOptionsProvider instance could be set on configuration (pretty much like you did in your pull request), and passed as optional argument in the view-model. The JvmDefault annotation is still required.

Do you think you can adjust the pull request? Otherwise, I can do it.

artudi54 commented 4 years ago

Sure, I can do it. I'll let you know when I'm done.

artudi54 commented 4 years ago

I moved color filter to new interface.

p-lr commented 4 years ago

Looking good, thanks!