Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

ModelInstanceCollection picking #4975

Merged
merged 4 commits into from
Feb 10, 2017
Merged

ModelInstanceCollection picking #4975

merged 4 commits into from
Feb 10, 2017

Conversation

lilleyse
Copy link
Contributor

@lilleyse lilleyse commented Feb 9, 2017

This fixes picking for ModelInstanceCollection. This doesn't affect i3dm picking which continues to work fine.

The picked object that is returned is a ModelInstance object which contains the instance's modelMatrix, the underlying collection, the underlying model, and its instance id.

You can set the modelMatrix to move the instance dynamically.

@freder Is this the sort of functionality you had in mind?

@lilleyse
Copy link
Contributor Author

lilleyse commented Feb 9, 2017

One test is failing in CI: culls when out of view and cull is true but passes locally with npm run test -- --browsers Electron --webgl-stub --failTaskOnError --suppressPassed...

@@ -54,6 +56,34 @@ define([
FAILED : 3
};

function ModelInstance(collection, modelMatrix, instanceId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be @private and in another file since it can be used outside of this file?

@pjcozzi
Copy link
Contributor

pjcozzi commented Feb 9, 2017

One test is failing in CI: culls when out of view and cull is true but passes locally with npm run test -- --browsers Electron --webgl-stub --failTaskOnError --suppressPassed...

Passes locally for me too. Looks unrelated. And I see different test failures in CI.

1) can create and destroy
     Widgets/CesiumInspector/CesiumInspector
     Expected HTMLNode to be HTMLNode.
    at Specs/Widgets/CesiumInspector/CesiumInspectorSpec.js:32:34
    at Object.<anonymous> (Specs/customizeJasmine.js:80:30)
2) can create and destroy
     Widgets/Geocoder/Geocoder
     Expected HTMLNode to be HTMLNode.
    at Specs/Widgets/Geocoder/GeocoderSpec.js:72:34
    at Object.<anonymous> (Specs/customizeJasmine.js:80:30)
     Expected 0 not to equal 0.
    at Specs/Widgets/Geocoder/GeocoderSpec.js:74:47
    at Object.<anonymous> (Specs/customizeJasmine.js:80:30)
3) mousedown event closes dropdown if target is not inside container
     Widgets/SceneModePicker/SceneModePicker
     TypeError: Cannot read property 'dispatchEvent' of null
    at fireMouseDown (Specs/DomEventSimulator.js:225:20)
    at Specs/Widgets/SceneModePicker/SceneModePickerSpec.js:54:13
    at Object.<anonymous> (Specs/customizeJasmine.js:80:30)
4) touchstart event closes dropdown if target is not inside container
     Widgets/SceneModePicker/SceneModePicker
     TypeError: Cannot read property 'dispatchEvent' of null
    at fireTouchStart (Specs/DomEventSimulator.js:246:20)
    at Specs/Widgets/SceneModePicker/SceneModePickerSpec.js:54:13
    at Object.<anonymous> (Specs/customizeJasmine.js:80:30)

@pjcozzi
Copy link
Contributor

pjcozzi commented Feb 9, 2017

Code is also OK with me, just that one comment.

@freder
Copy link

freder commented Feb 10, 2017

@lilleyse 👍

@lilleyse
Copy link
Contributor Author

Updated by putting ModelInstance into its own file and making sure to expand the bounding volume when an instance moves.

@pjcozzi
Copy link
Contributor

pjcozzi commented Feb 10, 2017

Part of #4897

@pjcozzi
Copy link
Contributor

pjcozzi commented Feb 10, 2017

Looks good!

@pjcozzi pjcozzi merged commit 43dc60c into 3d-tiles Feb 10, 2017
@pjcozzi pjcozzi deleted the mic-picking branch February 10, 2017 13:57
@pjcozzi
Copy link
Contributor

pjcozzi commented Feb 10, 2017

Thanks for the quick review, @freder!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants