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 pick with skyatmosphere #4790

Merged
merged 1 commit into from
Jan 4, 2017

Conversation

rutsky
Copy link
Contributor

@rutsky rutsky commented Dec 28, 2016

Add raising DeveloperError if previously picked object is picked again in drillPick.
Don't draw SkyAtmosphere when picking.

Fixes #4783 and #4784.

@rutsky
Copy link
Contributor Author

rutsky commented Dec 28, 2016

Please review changes thoroughly --- I'm not familiar with CesiumJS internals.

@rutsky
Copy link
Contributor Author

rutsky commented Dec 28, 2016

What is expected pickId when picking not-object (stars, Earth tiles)? Shouldn't Context.getObjectByPickColor get zero color for anything but not object?

Because I see that Context.getObjectByPickColor gets colors like the following:

4278190338
4278190081
4278255873
4278190081
4278190338

@hpinkos
Copy link
Contributor

hpinkos commented Dec 28, 2016

Thanks @rutsky! Most of the team is out of the office this week, but we'll try review this soon.


//>>includeStart('debug', pragmas.debug);
if (pickedResult === prevPickedResult) {
throw new DeveloperError(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would mask a bug elsewhere - basically setting a primitive's show to false not working. Can you remove this and keep the scope of this bug to picking the atmosphere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I excluded commit with adding DeveloperError from the pull request.

Are you sure there is no benefit in adding such internal consistency check? (I can open second PR with this change.)
IMO this check costs almost nothing, but can improve debugging experience next time when pick issue will arise.

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 28, 2016

@rutsky usually we would add a new test, but it will be hard to reasonably do so without tightly coupling the test to the engine code so don't worry about it.

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 28, 2016

What is expected pickId when picking not-object (stars, Earth tiles)?

Looking at the sky box, sun, and moon update functions they do not generate pick commands so they should not be pickable (they are handled outside our normal command processing so the color commands are still being executed in the pick pass, which is the problem).

The globe tiles won't generate any pick commands, other than writing depth, but they are processed in the right place so they are unlikely to be an issue.

I can finish this PR up next week if that is OK with you. Just address #4790 (comment) in the meantime, please.

@rutsky rutsky force-pushed the fix_pick_with_skyatmosphere branch from 149d540 to a7c2a64 Compare December 28, 2016 23:04
@rutsky
Copy link
Contributor Author

rutsky commented Dec 29, 2016

I can finish this PR up next week if that is OK with you. Just address #4790 (comment) in the meantime, please.

Thanks, @pjcozzi , it's better if you finish this PR, since it requires deeper knowledge of Cesium internals that I have, plus it's holidays here in Russia next week.

I removed commit with consistency check --- see my comment.

@pjcozzi pjcozzi mentioned this pull request Jan 4, 2017
@pjcozzi pjcozzi merged commit a7c2a64 into CesiumGS:master Jan 4, 2017
@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 4, 2017

This comment, #4783 (comment), was meant for this PR:

Thanks again for the fix, @rutsky. I made a small tweak to also not render the sun, moon, and sky box and merged everything in #4806 since I could not commit to your branch (could have been an issue on my end).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants