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

Improve Street controls 1 #1017

Merged
merged 6 commits into from
Feb 22, 2019
Merged

Conversation

gliegard
Copy link
Contributor

Description

improve StreetControls : add click and go, surface helpers, click and look at, tweens animations :

  • Mouse click on ground detection, and associated callback, that go to the clicked position
  • Mouse click on other object (not ground), and associated callback, that makes camera look at the position
  • Surfaces as helpers for the end user :
    • Circle displayed when user moves the mouse on the ground
    • Rectangle displayed when user moves the mouse on other objects (like walls of a building)
  • Tweens animations :
    • camera can move to another position smoothly
    • camera can look at a position smoothly in an animation

Motivation and Context

User should have a good experience using immersive example

Screenshots (if appropriate)

helper_wall
helper_ground

Copy link
Contributor

@gchoqueux gchoqueux left a comment

Choose a reason for hiding this comment

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

Nice improvement

src/Core/TileMesh.js Show resolved Hide resolved
src/Controls/StreetControls.js Outdated Show resolved Hide resolved
src/Controls/StreetControls.js Outdated Show resolved Hide resolved
src/Controls/StreetControls.js Outdated Show resolved Hide resolved
src/Controls/StreetControls.js Outdated Show resolved Hide resolved
src/Controls/StreetControls.js Show resolved Hide resolved
src/Controls/StreetControls.js Show resolved Hide resolved
src/Controls/StreetControls.js Outdated Show resolved Hide resolved
src/Controls/StreetControls.js Show resolved Hide resolved
src/Controls/StreetControls.js Outdated Show resolved Hide resolved
Copy link
Contributor

@zarov zarov left a comment

Choose a reason for hiding this comment

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

Could you add an explanation in the message of your fix commit ?

Anyway, very happy to see something like this, though it needs a bit more work. Thanks a lot !

src/Controls/StreetControls.js Outdated Show resolved Hide resolved
src/Controls/StreetControls.js Show resolved Hide resolved
src/Controls/StreetControls.js Outdated Show resolved Hide resolved
src/Controls/StreetControls.js Outdated Show resolved Hide resolved
src/Controls/StreetControls.js Outdated Show resolved Hide resolved
// Raycaster used to pick objects
this.raycaster = new THREE.Raycaster();
// Tween is used to make smooth animations
this.tweenGroup = new TWEEN.Group();
Copy link
Contributor

Choose a reason for hiding this comment

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

Declare this.tweens (which should be this.tween) here. Side note: why do you need a group and a tween ? (I know nothing about this library)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok. side answer: First I try without a Group but I didn't manage to make it work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I'll also take a look at it to see if we can make it work

// basic threeJs pick with raycaster
const normalized = this.view.viewToNormalizedCoords(event);
this.raycaster.setFromCamera(normalized.clone(), this.camera);
const intersects = this.raycaster.intersectObjects(this.view.scene.children, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

This will be very very heavy if you are inside a building with a complex mesh: we need a better solution. Now, I'm not going to block this PR for this, but I would appreciate if you already have some insights on how to fix this.

Anyway, please add a warning concerning this problem for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Finally, I use existing picking method, and pick on specific layers !!

src/Controls/StreetControls.js Outdated Show resolved Hide resolved
src/Controls/StreetControls.js Outdated Show resolved Hide resolved
src/Core/TileMesh.js Show resolved Hide resolved
@gliegard gliegard force-pushed the improve_street_controls_1 branch from 29073df to d905c71 Compare January 30, 2019 13:33
@gliegard gliegard added the wip 🚧 Still being worked on label Jan 30, 2019
src/Controls/StreetControls.js Outdated Show resolved Hide resolved
this.start = this.camera.clone();
this.end = this.camera.clone();
this.end.lookAt(position);
const factor = { t: 0 };
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I meant that you could directly write new TWEEN.Tween({ t: 0 }, this.tweenGroup) below.

// Raycaster used to pick objects
this.raycaster = new THREE.Raycaster();
// Tween is used to make smooth animations
this.tweenGroup = new TWEEN.Group();
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I'll also take a look at it to see if we can make it work

@gchoqueux gchoqueux added this to the 2.8.0 milestone Feb 1, 2019
@gliegard gliegard force-pushed the improve_street_controls_1 branch from 422fded to 3e37235 Compare February 1, 2019 16:29
@gliegard gliegard force-pushed the improve_street_controls_1 branch 2 times, most recently from 1767609 to 3845dab Compare February 19, 2019 14:56
@gliegard gliegard removed the wip 🚧 Still being worked on label Feb 19, 2019
examples/immersive_paris.html Show resolved Hide resolved
examples/immersive_paris.html Show resolved Hide resolved
src/Controls/StreetControls.js Outdated Show resolved Hide resolved
src/Controls/StreetControls.js Outdated Show resolved Hide resolved
src/Controls/StreetControls.js Outdated Show resolved Hide resolved
src/Controls/StreetControls.js Show resolved Hide resolved
src/Controls/StreetControls.js Show resolved Hide resolved
this.view.removeFrameRequester(MAIN_LOOP_EVENTS.BEFORE_RENDER, this.animationFrameRequester);
}

stopTweensAnimations() {
Copy link
Contributor

Choose a reason for hiding this comment

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

could you rename in stopAnimation? the user doesn't know it's tween.
could add this.view.removeFrameRequester(MAIN_LOOP_EVENTS.BEFORE_RENDER, this.animationFrameRequester); in stopTweensAnimations?

or merge onComplete and stopTweensAnimations...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I've renamed to stopAnimation. And I 've remove onComplete because it's not need to be exposed.

Copy link
Contributor

@gchoqueux gchoqueux Feb 21, 2019

Choose a reason for hiding this comment

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

this.view.removeFrameRequester isn't removed in all cases

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, thanks, updated

src/Controls/StreetControls.js Outdated Show resolved Hide resolved
src/Controls/StreetControls.js Outdated Show resolved Hide resolved
Copy link
Contributor

@zarov zarov left a comment

Choose a reason for hiding this comment

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

Overall it seems the quality downgraded. When i click on the street to move, it takes one second to start, and then it is not smooth at all.

}
}

onMouseMove(event) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I have an error when moving the mouse before the layer is ready. Could you add a condition like if (!layer.ready) ? Or something that fixes the error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, thanks, I 've added a bugfix in View

@gliegard gliegard force-pushed the improve_street_controls_1 branch from 277f54d to c51b59e Compare February 21, 2019 10:45
@gliegard gliegard added the ready label Feb 21, 2019
…ok at, tweens animations :

- Mouse click on ground detection, and associated callback, that go to the clicked position
- Mouse click on other object (not ground), and associated callback, that makes camera look at the position
- Surfaces as helpers for the end user :
  - Circle displayed when user moves the mouse on the ground
  - Rectangle displayed when user moves the mouse on other objects (like walls of a building)
- Tweens animations :
  - camera can move to another position smoothly
  - camera can look at a position smoothly in an animation
…r not ready.

Before this commit, picking on a layer, that was not yet ready, thrown an exception.
To see an example, look at immersive_paris.html example, you can change building layer transparency in the debug UI.
Before this commit, orientedImageCamera, has a default far set to 1000,
A side effect is that OrientedImageMaterial was only able to project texture to a distance of 1000,
because OrientedImageMaterial uses OrientedImageCamera to project textures.
Now, OrientedImageCamera default far set to 10 000, so OrientedImageMaterial projects texture to 10 km distance.
…gs' layer to 'Buildings', and increase background radius from 80 to 1 200
@gliegard gliegard force-pushed the improve_street_controls_1 branch from 4f46db4 to 513ad4a Compare February 21, 2019 14:26
Copy link
Contributor

@gchoqueux gchoqueux left a comment

Choose a reason for hiding this comment

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

thanks

@gchoqueux gchoqueux merged commit 89fef54 into iTowns:master Feb 22, 2019
@gliegard gliegard deleted the improve_street_controls_1 branch March 29, 2019 10:45
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants