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

2D or 3D Rendering according to pano.mode instead of only for chrome #26

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

vanoostrum
Copy link

First off al thanks for writing this panomarker plugin, it's very useful! I have been starting to use it with chrome and everything worked perfectly, but when I tried to use it with firefox I saw that the positioning wasn't accurate anymore. I Found out this was because I'm using the StreetViewPanorama.mode setting to force firefox to render the StreetView Panorama in webgl mode, meaning rendering in a 3D Sphere. The problem is that your plugin doesn't take this mode setting into account and just assumes that only chrome uses webgl. I made some changes to the code by adding a check for the pano.mode property. Although it wouldn't work if pano isn't set in the properties. So maybe the check should be done at runtime (on pov_changed). Maybe you can review my change and if you like the solution merge it or change it into something you think works. Thanks!

@marmat
Copy link
Owner

marmat commented Nov 1, 2015

Thanks so much for your change and sorry for the huge delay, somehow I missed the Github notification. Small nit: could you update the minified version as well?

@vanoostrum
Copy link
Author

I will take a look at it soon, and do some more testing to make sure the code is correct

@vanoostrum
Copy link
Author

You were right about the case when mode is not explicitly set. I added an extra check for that. Also there's the case when the pano is not initially set, but later using panoMarker.setPano(). In that case the check needs to be done in the setPano() method. Since this method is also called in the constructor, I moved the setting of povToPixel_ to there. Hope you think this solution is ok. I also minified the js (using http://jscompress.com/)

@vanoostrum
Copy link
Author

Then the logic to determine whether to use 2d or 3d should be put in some separate function, which can be called from the constructor and from the setPano method. Otherwise this code needs to be copied, which is not good. Do you have any suggestions how to set this login in a (private) method?

@marmat
Copy link
Owner

marmat commented Nov 29, 2015

Since we call setPano from the constructor anyway, you could simply set a reasonable default value in the constructor instead of copying the logic, e.g.:

/**
 * Rendering ...
 */
this.povToPixel_ = PanoMarker.povToPixel3d;

and in setPano only:

this.povToPixel_ = opts.pano !== null && (opts.pano.mode === "webgl" || opts.pano.mode === "html5") ?
    PanoMarker.povToPixel3d :
    PanoMarker.povToPixel2d;

@gurmukhp
Copy link
Contributor

gurmukhp commented Dec 2, 2015

Would it be possible to define a helper method that returns whether a 3D or 2D renderer is used? An alternative way to check which renderer to use could be to check if a canvas element is in the container, if so, then it will be 3D, otherwise it'll be 2D.

@vanoostrum
Copy link
Author

I made some changes hoping this long standing pull request will be picked up. Now checking whether to use 2d or 3d by looking for a canvas element in the container and checking getContext().
I put a method is3D in the constructor, not sure if this is the correct place for it. @marmat could you have a look at this and let me know if it is acceptable to merge?

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.

4 participants