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

Remove unused/unfinished classes #1000

Merged
merged 7 commits into from Mar 22, 2019

Conversation

Projects
None yet
4 participants
@tsherif
Copy link
Member

commented Mar 22, 2019

Remove Sampler, Texture2DArray, pickModels, UniformBufferLayout.

Moved code, docs and specs into 'wip' directory.

@tsherif tsherif requested review from ibgreen and Pessimistress Mar 22, 2019

@coveralls

This comment has been minimized.

Copy link

commented Mar 22, 2019

Coverage Status

Coverage increased (+0.2%) to 46.389% when pulling 091b72e on chopping-block into 2fe6b90 on master.

@ibgreen

This comment has been minimized.

Copy link
Contributor

commented Mar 22, 2019

The most important parts of this PR is to explain what is happening in Upgrade Guide, please add that.

Also make sure you have run all examples and that they still work

UniformBufferLayout - I'd really like to keep the UniformBufferLayout for now, as I see that as a big part of the near-term evolution of the framework as discussed yesterday. It does have test cases and in that sense is not an untested piece of code, so I'd like to have a thorough discussion of why that class does not have a place in the next release before cutting it, and I prefer we take that discussion after 7.0 release.

pickModels - I've gone back and forth about this, but Im still not happy about pickModels. pickModels was already commented out in the examples which leaves us with another mess (having commented non-functional code in examples). We could cut that code now in all the examples but then we have to add it all back in the next release, and that is a level of churn that I do not appreciate. I am really tempted to just restore pickModels the way it worked in 6.4 (a five line change) and leave it in for now, and then fix it properly by porting some of luma.gl picking in next release.

Finally there is some is additional WIP code inside e.g. core and webgl modules, suggest we also move that to the new top-level wip folder in this PR.

@tsherif

This comment has been minimized.

Copy link
Member Author

commented Mar 22, 2019

I'd really like to keep the UniformBufferLayout for now

Do you think we could update one of the examples to use it? That would be enough to make me more comfortable with its inclusion.

was already commented out in the examples which leaves us with another mess
(having commented non-functional code in examples)

My preference here would be for re-implementing picking in the examples manually, by just reading the pixels, then maintaining the previous code that wasn't really working.

additional WIP code inside e.g. core and webgl modules

Which ones were you thinking?

@ibgreen

This comment has been minimized.

Copy link
Contributor

commented Mar 22, 2019

Which ones were you thinking?

Found only this one https://github.com/uber/luma.gl/tree/master/modules/core/wip

My preference here would be for re-implementing picking in the examples manually

Sounds good!

Do you think we could update one of the examples to use it

Great idea, maybe the transform feedback example as it is already webgl2?

@@ -18,6 +18,10 @@ A luma.gl <code>Cube</code>, rendering 65,536 instances in a
single GPU draw call using instanced vertex attributes.
`;

function getDevicePixelRatio() {

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 22, 2019

Contributor

I think the animationLoop provides this as a param

This comment has been minimized.

Copy link
@tsherif

tsherif Mar 22, 2019

Author Member

It provides the true/false param, but not the device pixel ratio itself: https://github.com/uber/luma.gl/blob/master/modules/core/src/core/animation-loop.js#L286-L308

(Maybe it should?)

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 22, 2019

Contributor

Yes, I think it should, in fact the ratio can very per frame (e.g. when zooming into a web page) so why not send it in every frame?

tsherif added some commits Mar 22, 2019

@tsherif tsherif merged commit e22e541 into master Mar 22, 2019

4 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.2%) to 46.389%
Details
license/cla Contributor License Agreement is signed.
Details

@tsherif tsherif deleted the chopping-block branch Mar 22, 2019

@tsherif tsherif referenced this pull request Apr 11, 2019

Closed

Verify and fix `UniformBufferLayout` class. #484

3 of 8 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.