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

Further trackball fixes #3757

Closed
wants to merge 2 commits into from
Closed

Conversation

kevinoe
Copy link
Contributor

@kevinoe kevinoe commented Aug 14, 2013

@bsenftner - I think I finally have a fix for scrolling. Could you test?

Previously the screen variable was not updated on scroll, which meant that screen.top & screen.left could be wrong. This updates those values before access in the mouse events.

The position relative to the domElement when scrolling is taken into consideration is then

event.pageX - screen.left, event.pageY - screen.top

I also added a snippet to calculate pageX/Y if needed

BUGZID:
Summary:
Previously the screen variable was not updated on scroll.
This updates it before every request.

The relative position is then

event.pageX - screen.left, event.pageY - screen.top

I also added a snippet to calculate pageX/Y if needed
@WestLangley
Copy link
Collaborator

This produces odd behavior for me on OSX -- so odd, I can't even describe it. Perhaps my test case is not what you intended. What is your test case?

Also, what do you feel is the compelling reason for adding this feature?

@kevinoe
Copy link
Contributor Author

kevinoe commented Aug 14, 2013

@WestLangley - So my original fix (which was previously merged) was intended to make the trackball only map to the actual domElement (versus the full screen as it was before). However, in cases when the page was scrolled, the trackball was not "centered" in the threejs domElement any more.

You can't describe it at all? What browser?
I've tested with:
Chrome 28 on OSX,
Safari 6.0.5 on OSX
IE10 on Windows
Chrome28 on Windows
Firefox 20 on Windows

I've used two of my internal pages, and the Trackball demo (examples/misc_controls_trackball.html)

@WestLangley
Copy link
Collaborator

With all due respect, you failed to answer my question. What is your test case?

@kevinoe
Copy link
Contributor Author

kevinoe commented Aug 14, 2013

@WestLangley - I have a page (actually multiple) that has a three.js element off screen (in a independently scrollable div). I scroll down, place a breakpoint in the debugger on getMouseProjectionOnBall and click in the multiple corners to ensure the values map correctly to the range -1 to 1.

I'd still like to know what you are seeing and in what framework. Are you having completely invalid rotations? Does it jump erratically? Is the code even running?

@WestLangley
Copy link
Collaborator

To test your change, I modified misc_controls_trackball.html by replacing window.innerHeight with ( 2 * window.innerHeight ) everywhere, and removed body { overflow: hidden }.

The trackball motions are unusual after scrolling to the bottom of the page.

@kevinoe
Copy link
Contributor Author

kevinoe commented Aug 14, 2013

Awesome - I hadn't considered a case where the three.js element was greater than the viewport size. I will go try & see what I find.

@kevinoe
Copy link
Contributor Author

kevinoe commented Aug 14, 2013

@WestLangley - Are you seeing a "roll" behavior?

@kevinoe
Copy link
Contributor Author

kevinoe commented Aug 14, 2013

Ok - here's my analysis:

  1. The original three.js code assumed the full viewport was the trackball (https://github.com/mrdoob/three.js/blob/master/examples/js/controls/TrackballControls.js#L74)
  2. My code took scrolling into account.
  3. Because of SVG support? #1, the "screen" variable is the size of the viewport
  4. I'm however using the full page size which is now greater than the size of the viewport.

So I see two possible fixes:

  1. Ignore the scrolling in the case when this.domElement == document
  2. Change the screen.width/screen.height to be the document width and height, not the window width and height. This could result in trackballs larger than the window.

Both could yield odd behaviors. In 1, the trackball would map to the window, but the rotation would not "match" the mouse movement. In 2, the trackball is larger than the window, so the center of the trackball might not be in the center of the window (or even window at all), but I think the mouse dragging would match better.

(Hopefully that made sense. If need be, I can make & upload an illustration)

@kevinoe
Copy link
Contributor Author

kevinoe commented Aug 14, 2013

The viewport is the grey square. The trackball is the red circle.


Case 1
globe_window_trackball


Case 2
globe_page_trackball

@kevinoe
Copy link
Contributor Author

kevinoe commented Aug 14, 2013

I'm inclined to say that if they install the trackball on the window, ignore scrolling. If they want the "large trackball" behavior, it can be created by installing the trackball on the actual canvas domElement.

Does that seem reasonable?

@kevinoe
Copy link
Contributor Author

kevinoe commented Aug 15, 2013

@WestLangley Does this look any better?

@WestLangley
Copy link
Collaborator

I'd like to wait until @mrdoob decides if this is a feature he wants to support.

Also, please remember that it is your responsibility to demonstrate that your code works. That is why I asked to see your test cases, and by that, I meant live test cases -- not a description of your test cases. I could have made myself more clear.

@kevinoe
Copy link
Contributor Author

kevinoe commented Aug 15, 2013

This is really more of a fix for something that is already there, but not quite working when scrolled. If I were to create a live test case, where would I put it? I'm happy to write something up. I think one of the main confusions I run into with this is that I'm not doing a full-window THREE.js instance (which seems to be what most people do?) Instead, I'm doing smaller views embedded into a page.

Thanks for all your input on this - I am new here, but definitely want to help & contribute as best I can.

@WestLangley
Copy link
Collaborator

Yes, I had a difficult time trying to understand what this PR was all about.

You could open a dropbox account and put your example in the public folder. You could also include a new example in the PR if you wanted to. In fact, that is what is suggested in the the Wiki article How to Contribute to three.js -- although it is true that people rarely do that, and I wouldn't expect it in this case.

As far as feedback on your code goes, I do not have the hardware/OS platforms for testing it, so I need to defer to others in that regard.

@kevinoe
Copy link
Contributor Author

kevinoe commented Aug 15, 2013

Thanks - I'm happy to put an example in, but it seemed a bit like overkill. Perhaps THREE.js could consider a "tests" directory in the future. I'll see if I can get a dropbox example going & then post the link here. If it makes sense to move it into the examples after that, I can.

As a bit of background - this is a followup PR from this one:
#3474

based off discussions from this PR with @bsenftner and @mrdoob (an earlier version):
#3411

Thanks for your help and I'll post a link when I get it up & running.

@kevinoe
Copy link
Contributor Author

kevinoe commented Aug 15, 2013

https://dl.dropboxusercontent.com/u/20572495/three.js/examples/misc_controls_trackball2.html

This is an example of a small, embedded THREE.js instance in a scrollable window.

@mrdoob
Copy link
Owner

mrdoob commented Aug 15, 2013

@WestLangley Thanks for guiding @kevinoe.

@kevinoe Could you also add another small three.js instance on that page but using the current controls (so it's easy to see the improvement).

@WestLangley
Copy link
Collaborator

OK, now I understand what this is all about. Thank you for the live example.

As I explained before, I'll leave it up to others to comment on your implementation approach.

@kevinoe
Copy link
Contributor Author

kevinoe commented Aug 15, 2013

@mrdoob - I'll see what I can do.

@bsenftner
Copy link

@kevinoe Sorry for my delay in trying out this version in my setup. I grabbed a copy of the TrackballControls.js from the above dropbox hosted example, and tried in a test page where there is a 460x500 Three.js canvas on the left side of a page, beneath a 214px high header div, and on the right side, next to the Three.js canvas is an equally sized Raphael.js canvas:

trackballissue1

The Three.js canvas is outlined in red.

trackballissue2

The same issue still exists, anytime the top of our Three.js canvas is partially scrolled off the top of the browser window, the TrackballControls revert to the z-rotation only. The above image is a z-rotation only situation.

I tried to see if the same behavior is exhibited from your dropbox example by shrinking my browser window so the Three.js canvas could be partially scrolled off the top. Your dropbox hosted version works. The only difference I can see is that your using Three.js R60, where I'm running Three.js R59. Are there changes between the two versions that could account for this difference?

I also noticed in your Dropbox version, when the Three.js canvas is partially scrolled off page the Three.js instance will disappear after 10-40 seconds with no console warnings or errors. This also occurs when changing the browser window size. This is running Chrome 28.0.1500.95 and FireFox 23 on OSX 10.7.5.

@kevinoe
Copy link
Contributor Author

kevinoe commented Aug 15, 2013

I just updated the example to include an old instance in the left column.

@bsenftner - I will add a bunch of text below as well, and make them a bit larger to test for this.

@kevinoe
Copy link
Contributor Author

kevinoe commented Aug 15, 2013

@bsenftner - Added. Not seeing what you are seeing. I started off the dev branch, but I'm not sure what else would be different..

@kevinoe
Copy link
Contributor Author

kevinoe commented Aug 15, 2013

I saw the disappearing issue when I resized the window. The webpage had some bugs (not the THREE.js library or Trackball code). Should be fixed.

@bsenftner
Copy link

I just tried your new version on Dropbox. It appears to be working. I tried with the three.js top partially off the top of the window, with it partially scrolled off the bottom, and with the browser window small so it only shows a center portion - all four sides partially clipped - and it appears to work.
...which leads me to think I'm stepping on something somewhere in my logic... oh fun!

Thanks for your persistent @kevinoe I appreciate it.

@kevinoe
Copy link
Contributor Author

kevinoe commented Aug 15, 2013

@bsenftner The one thing I'd check is what domElement you have the Trackball installed. The default is the window, not the domElement. I'd be happy to help if your site is live.

@bsenftner
Copy link

@kevinoe Strange I can't find a way to private message you with contact details. I'm bsenftner on Skype. Ping me and I'll give you temp access to our DEV setup where we can investigate this Trackball issue deeper.

@kevinoe
Copy link
Contributor Author

kevinoe commented Aug 16, 2013

@bsenftner - I sent you a Skype contact request. Not sure I can do it today, but let me know when works. Also added my email to my profile here on github

this.screen.width = window.innerWidth;
this.screen.height = window.innerHeight;

} else {

this.screen = this.domElement.getBoundingClientRect();
Copy link
Contributor

Choose a reason for hiding this comment

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

What about when the dom element isn't document? I tried to fix this case independently in #4151 -- it might be useful to compare these two pull requests.

@mrdoob mrdoob closed this Jan 12, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants