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

Fix Geometry in Firefox #1042

Merged
merged 2 commits into from
Aug 20, 2013
Merged

Fix Geometry in Firefox #1042

merged 2 commits into from
Aug 20, 2013

Conversation

bagnell
Copy link
Contributor

@bagnell bagnell commented Aug 20, 2013

Change Enumeration reference equality checks to perform a value comparison. Work around transferable object bug in Firefox:

https://bugzilla.mozilla.org/show_bug.cgi?id=841904

This fixes #1039.

…rison. Work around transferable object bug in Firefox.
@mramato
Copy link
Contributor

mramato commented Aug 20, 2013

This is more of a question for @shunter. Do we expect to always have to do value comparisons for enums in webworkers? It kind of negates the point of having Enumeration be an object rather than just a constant value, doesn't it? It's also really easy to write code that appears to run fine (and works under Chrome) but then fails in Firefox. If this is temporary, fine; but if now, maybe we should rethink our enum strategy? Just a thought.

@bagnell
Copy link
Contributor Author

bagnell commented Aug 20, 2013

We always have to do value comparisons for enums transferred to or from a
worker. Its the same in both Firefox and Chrome. The reason it was failing
in Firefox is because the extension for unsigned int indices was
unavailable. The function to fit the indices to unsigned shorts was being
called in Firefox and not Chrome.
On Aug 19, 2013 10:34 PM, "Matthew Amato" notifications@github.com wrote:

This is more of a question for @shunter https://github.com/shunter. Do
we expect to always have to do value comparisons for enums in webworkers?
It kind of negates the point of having Enumeration be an object rather
than just a constant value, doesn't it? It's also really easy to write code
that appears to run fine (and works under Chrome) but then fails in
Firefox. If this is temporary, fine; but if now, maybe we should rethink
our enum strategy? Just a thought.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1042#issuecomment-22918875
.

@mramato
Copy link
Contributor

mramato commented Aug 20, 2013

I can confirm that the tests all pass now and Firefox Sandcastle examples work as expected. As an aside, is there any sort of event that gets fired as geometry is processed? It would be useful to put up some sort of loading indicator for the G&A Sandcastle example, it's really a dog on Firefox.

I'll let @shunter do the actual merge.

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 20, 2013

maybe we should rethink our enum strategy?

I have agreed with this for a while, and in #1023, @shunter is also starting to consider it. Propose something on the forum or just open an issue.

As an aside, is there any sort of event that gets fired as geometry is processed?

Good idea. We'll put it on the roadmap, but it may be a bit before we get to it.

if (attributes.hasOwnProperty(name) &&
defined(attributes[name]) &&
defined(attributes[name].values) &&
transferableObjects.indexOf(attributes[name].values.buffer) < 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to convert the typed arrays even if the ArrayBuffer is already in transferableObjects.

@bagnell
Copy link
Contributor Author

bagnell commented Aug 20, 2013

@shunter This is ready for another review.

shunter added a commit that referenced this pull request Aug 20, 2013
@shunter shunter merged commit 2dfdb97 into master Aug 20, 2013
@shunter shunter deleted the geometryWebWorker branch August 20, 2013 21:00
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.

Geometry broken in Firefox
4 participants