Pull request for cameraChange event and DomEvent adjustment #189

Closed
wants to merge 2 commits into
from

Projects

None yet

2 participants

@leconcepteur

Added the event cameraChange, triggered when the camera is changed.
Adapted the tQuery domevent to use the cameraChange event.
Corrected threex.domevent.js to be compatible with all types of Three.js cameras.

  • The "THREE.CSG" part of the code in tquery-all.js hasn't changed. I guess it's the EOL character that changed but I wasn't able to keep the original.
@leconcepteur leconcepteur Added the capability to change the tQuery camera.
Added the event cameraChange, triggered when the camera is changed.
Adapted the tQuery domevent to use the cameraChange event.
Corrected threex.domevent.js to be compatible with all types of Three.js cameras.
c3b8a16
@leconcepteur

I'm not sure I did it right as it's my first Pull Request.
I changed the the file "threex.domevent.js" but I can fork the Threex project if you prefer.
Also, I did the request on the master branch, should I do it on the dev branch instead?

@leconcepteur leconcepteur commented on the diff Aug 8, 2012
build/tquery-all.js
@@ -5731,116 +5755,116 @@ CSG.Node.prototype = {
}
};
@leconcepteur
leconcepteur Aug 8, 2012

I didn't touch at all the part below. Maybe it's the EOL. I can't figure it out.

@jeromeetienne
Owner

cool! it seems good at first sight :)

yeah dont worry about the tquery-all / tquery-bundle ... i should not put that in the git in itself...

i will look and merge tomorrow night. im out of town tomorrow

@jeromeetienne

why did you change the method for rays ?

Owner

There was the issue #599 of Three.js for the problem of the intersecs when using an OrthographicCamera.
In his last comment, jsermeno was telling that he used pickingRay instead, and that it works with both orthographic and perspective cameras.
mrdoob/three.js#599 (comment)

The tests I did showed that it worked well, but you're better placed than me to know if I'm missing something.

perfect lets change then. you seems to know more than me on this :)

Owner

Merci, je vais utiliser le DomEvent intensément. Il ne devrait pas y avoir de problèmes mais si jamais j'en trouve, je ferai un autre Pull Request pour le régler.

good! hit it :)

@jeromeetienne

i dont understand this. tQuery.World is the class.. and you bind an event on it ?

Owner

I bind the event to listen at what is triggered at line 1292 of tquery.js...

@jeromeetienne

i see i was the one suggesting this ... not too good of a suggestion :) i will think about this domevent+camera issue tomorrow

Owner

Yes, sorry, I was trying to follow your train of thought. I think it'd be best if you find something that suits you. It's working for now, that's all I need :)

hmm the fact it is working is a worry :) it shouldnt. this implies there is another bug somewhere :(

glad you are unblocked tho

@jeromeetienne
Owner

thinking about this... several observations thinking out loud

  • threex.domevent is specific to a given THREE.Camera
  • in tQuery, this means that object3d.on('click', function(){}); need to know the current camera
  • in jQuery and DOM, it is easy as they got document.body
  • in tQuery this is document.body is tQuery.world
@jeromeetienne
Owner

possible solution ?

  • have a tQuery.World event cameraChange to handle camera change
  • do a world.enableDomEvents() and .disableDomEvents()
    • store the threex.domevent in the world instance
  • api mesh.on('click', function(){ /* */ }, world);
    • with world being optional. if not provided, use tQuery.world
@jeromeetienne
Owner
mesh.on('click', function(){ /* */ }, world);

with world optional. @leconcepteur what do you think ? would that fly ? (in anycase code away, the api wont change in any significative way)

@leconcepteur

It sounds good to me!

@jeromeetienne
Owner

0d8b1dd i completed the modif you made. there are all your modification but the change of camera event. i didnt like it when i coded this in the train this morning. was changing camera important in your case ?

https://github.com/jeromeetienne/tquery/blob/master/plugins/domevent/tquery.domevent.js here is the new version.

@leconcepteur

No, you're right. I might do a plugin if I really need to change the camera.
I like the way you changed the DomEvent.

Merci

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