-
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
Sun calculator #655
Sun calculator #655
Conversation
|
||
public ExtrusionRenderer(boolean mesh, boolean translucent) { | ||
mMesh = mesh; | ||
mTranslucent = translucent; | ||
|
||
mSun = new Sun(); |
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.
Quick comment: since the feature is optional, better have a lazy initialization of any feature objects, specially if they carry collections.
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.
You mean lazy init the sun, or the collection?
Probably can achieve lazy init collection when calling updateColor()
the first time.
Yes it's a feature, but fully customizable. Storing in another attribute next to the sun would be confusing.
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.
The Sun
class.
Can use getSun
everywhere and if null at first call, just create it there.
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.
But we have to specify light position in any case?
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.
Aren't shadows optional?
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.
All parameters in sun are optional. So why make an option of an option, which in fact gives the same results and only makes it more complicated.
Yes this whole construct is needed for shadows, too. But also gives new options for extrusion renderer only.
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 don't think storing 5 values more has impact on performance or memory. It's a more physical based way to specify lights position.
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.
Let's wait to be reviewed then. Most concern is with users who simply don't care for light or shadows and want to continue with same fast rendering without new possible GL issues.
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 don't see any regression here. If somebody don't care for new features, can stay on tested versions.
Regressions can be seen with extensive testing. 🙂
That cannot be an answer to users: if don't need a feature, use previous lib version. |
Of course, there always can be an issue. But in this case the position parameter only moved to
You're right, but not all implementations can be optional, only if they offer a different and reasonable result. And I added a bunch of methods to make it all customizable. So e.g. can set the position simply via BTW: I removed one TODO...already working. |
Thanks, merged via eb3efa7. |
I also added the above test in |
Nice thanks! |
Sun calculator with accuracy of ~1 minute.
Can test actual sun position with:
BuildingLayer.getExtrusionRenderer().enableCurrentSunPos(true);
Or to see the whole day cycle use this in
MapsforgeTest.createLayers()
:See #575.