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

Fix for hill shading issues on Android 8+ #1131

Closed
wants to merge 2 commits into from

Conversation

@Sublimis
Copy link

commented Sep 25, 2019

Canvas.save() and restore() calls (being made on Android API 28 and up) cause intermittent hill shading issues: After zooming the map in/out a few times, some tiles (somewhat randomly chosen) loose their hill shading. Tested on Android 6 (which had no problems), 8, and 9. Removing those calls fixes the issue, and apparently without any side-effects.

Canvas.save() and restore() calls (being made on Android API 28 and up) cause intermittent hill shading issues: After zooming the map in/out a few times, some tiles (somewhat randomly chosen) loose their hill shading. Tested on Android 6 (which had no problems), 8, and 9. Removing those calls fixes the issue, and apparently without any side-effects.
@devemux86

This comment has been minimized.

Copy link
Collaborator

commented Sep 25, 2019

Canvas.clipRect(float, float, float, float, Region.Op) was deprecated in API level 26.

Instead of revert Android 9 compatibility, better fix any issues with new Android API.

Sorry, my IDE never warned me about deprecation. Now I simply removed only calls to canvas.save() and restore(), which also fixes the issue for me (again apparently without side-effects). If this simple workaround is not acceptable for other reasons, I think much more complicated and deeper code changes would be needed to address this.
@devemux86

This comment has been minimized.

Copy link
Collaborator

commented Sep 25, 2019

Sorry, my IDE never warned me about deprecation. Now I simply removed only calls to canvas.save() and restore(), which also fixes the issue for me (again apparently without side-effects). If this simple workaround is not acceptable for other reasons, I think much more complicated and deeper code changes would be needed to address this.

Something else is needed or changes in hillshading.
The removed lines break map rendering, e.g. zoom in.

Can find more details for the deprecated method online: https://stackoverflow.com/a/50247323

@Sublimis

This comment has been minimized.

Copy link
Author

commented Sep 28, 2019

Thanks for the link, in fact saw it prior to posting. Problem with the current code is that the following is basically a no-op instruction:

canvas.save();
canvas.clipRect(...);
canvas.restore();

It has no effect whatsoever: restore() operation immediately reverts any effect that clipRect(...) might have had.

In any case, the proposed workaround (removing canvas.save() and canvas.restore() calls from the code like the example above) seems to work for us. We're not using org.mapsforge.map.android.view.MapView, but instead rely on 3rd party library to draw the tiles so this might be the reason. Maybe this will help someone.

@devemux86

This comment has been minimized.

Copy link
Collaborator

commented Sep 28, 2019

That is what was needed so that vector map rendering works as before in Mapsforge.
Cannot comment for third-party rendering, as it's out of library's interest and workflow.
Every test needs be done internally first. Does HillshadingMapViewer sample works?

devemux86 added a commit that referenced this pull request Sep 28, 2019
@devemux86 devemux86 added the bug label Sep 28, 2019
@devemux86 devemux86 added this to the 0.13.0 milestone Sep 28, 2019
@devemux86

This comment has been minimized.

Copy link
Collaborator

commented Sep 28, 2019

I enhanced the graphics API and fixed it via different calls for hillshading via 6ff0d3f.

@devemux86 devemux86 closed this Sep 28, 2019
@devemux86 devemux86 changed the title Fix for hill shading issues on Android API 28+ Fix for hill shading issues on Android 8+ Sep 28, 2019
@Sublimis

This comment has been minimized.

Copy link
Author

commented Sep 28, 2019

Thanks for your efforts! But, not sure we're on the same page. Most important thing to notice is that this:

    @Override
    public void setClip(int left, int top, int width, int height, boolean save) {
        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) {
            if (save) {
                this.canvas.save();
            }
            this.canvas.clipRect(left, top, left + width, top + height);
            if (save) {
                this.canvas.restore();
            }
        } else {
            this.setClipInternal(left, top, width, height, Region.Op.REPLACE);
        }
    }

is semantically equivalent to this (UPDATE: when the save flag is true):

    @Override
    public void setClip(int left, int top, int width, int height, boolean save) {
        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) {
            if (save) {
                // DO NOTHING!
            }
            else {
               this.canvas.clipRect(left, top, left + width, top + height);
            }
        } else {
            this.setClipInternal(left, top, width, height, Region.Op.REPLACE);
        }
    }

So basically when the save flag is true, nothing happens on API 26+. More clarification here: https://stackoverflow.com/questions/29040064/save-canvas-then-restore-why-is-that.

The easiest solution to make clip bounds enlarging possible would be to save original clip bounds when Canvas is created, then restore them back as needed. For this to work, setBitmap() method shouldn't be part of org.mapsforge.core.graphics.Canvas interface. Instead, new canvas should be created when bitmap change is needed. Also, the no-parameter constructor shouldn't exist, and only the one that accepts Bitmap should be present.

If you can get rid of the aforementioned org.mapsforge.core.graphics.Canvas.setBitmap() method and the no-parameter constructor, we can do the rest of coding to implement this properly.

A sidenote, our project uses mapsforge to render tiles, of course. Third-party library is used only to show those tiles (as bitmaps) on the screen.

@devemux86

This comment has been minimized.

Copy link
Collaborator

commented Sep 28, 2019

is semantically equivalent to this:

No, it's not, how could be?
2nd breaks the zoom, mentioned that, have you tested it?
Also cannot understand what you mean with all other new changes.

Hillshading was reporting not working, was confirmed with HillshadingMapViewer sample on Android 8+ and was fixed.

A sidenote, our project uses mapsforge to render tiles, of course.

That's not mapsforge purpose.
If there is something to help with the public API we can examine it, but we cannot rewrite the library with that purpose.

@Sublimis

This comment has been minimized.

Copy link
Author

commented Sep 28, 2019

No, it's not, how could be?

Please check the link provided in our last reply. Restore reverts clipRect, as it can also be seen from the official documentation:

image

@devemux86

This comment has been minimized.

Copy link
Collaborator

commented Sep 28, 2019

So you propose at every clipRect with Region.Op.REPLACE on Android 8+ to not call anything?

@Sublimis

This comment has been minimized.

Copy link
Author

commented Sep 28, 2019

So you propose at every clipRect with Region.Op.REPLACE on Android 8+ to not call anything?

No; on Android 1 to 25 this works as intended, as Region.Op.REPLACE is supported there. I'm talking about Android 26+. The code above doesn't have any effect on Android 26+ whatsoever when the save flag is true. The code first checks that it's running on Android 26+; when it is (first branch of the if) and the save flag is true, it saves the clip, modifies the clip, then immediately restores the clip. This is a no-op.

@devemux86

This comment has been minimized.

Copy link
Collaborator

commented Sep 28, 2019

Mean changes like in branch hillshade?

@Sublimis

This comment has been minimized.

Copy link
Author

commented Sep 28, 2019

Mean change like in 4b543dc?

This change just simplifies the code, but unfortunately it behaves exactly the same. The intention was for you to see that the whole approach is (most probably) futile on Android 26+. Even if it works now, it will give undesirable effects as soon as someone (i.e. rest of the library code) tries to enlarge clip bounds, or calls AndroidCanvas.setBitmap() with bitmap of different dimensions. Can you somehow assert that this won't happen?

@devemux86

This comment has been minimized.

Copy link
Collaborator

commented Sep 28, 2019

We can only test and support via the mapsforge-samples-android (or desktop).
Hillshading was verified there with issues on Android 8+ and should work now.

For anything more related to API, can explain in detail in forum and see if improvements can be made without changing library behavior.

@Sublimis

This comment has been minimized.

Copy link
Author

commented Sep 28, 2019

Ok, thank you for now. We'll see what more can we do about it in the future!

@devemux86

This comment has been minimized.

Copy link
Collaborator

commented Sep 29, 2019

I merged the changes in master via 90be199.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.