-
Notifications
You must be signed in to change notification settings - Fork 91
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
Shadow for buildings #668
Shadow for buildings #668
Conversation
Co-authored-by: Izumi Kawashima <[email protected]>
Co-authored-by: Izumi Kawashima <[email protected]>
Exceptional result and you know that I am strict in PR reviews! I like the Merged via b4a98ef. |
@@ -319,7 +347,8 @@ public void render(GLViewport v) { | |||
} | |||
|
|||
/* just a temporary reference! */ | |||
ebs[i] = null; | |||
/* But for shadows we use them multiple times */ | |||
//ebs[i] = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we keep here the null set as before, when not using shadows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That needs an extra local variable. E.g. such as "mMultipleRenders". But is this really needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In any case mShader
is available and could use a check like in line:373?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works, but not the cleanest solution :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested with Android Studio Profiler and I couldn't notice any remarkable memory regression when holding ExtrusionBuckets
in ebs
. If really needed, we should null all not used entries of mExtrusionBucketSet
in BuildingRenderer.update
after new ExtrusionBuckets
are assigned (use activeTile
count to null the rest).
for(int i = activeTiles; i < mExtrusionBucketSet.length && mExtrusionBucketSet[i] != null; i++) {
mExtrusionBucketSet[i] = null;
}
But may it's not even needed or even costs more performance to explicitly nulling them, e.g. tags of MapElement
aren't nulled either.
See #575.
Activate with
BuildingLayer.SHADOW = true;
Troubleshooting:
OffscreenRenderer
not works together withShadowRenderer
yet. Colors have to be cleared correcltly and must be mixed in another way.ExtrusionRenderer
uses the callebs[i] = null;
this must be removed to allow render buildings twice: once from light, once from users view; may even is not necessary (thanks to garbage collecting)ExtrusionRenderer.setMatrix()
Have fun :)