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

perf: only do expensive reload when texture changes #10684

Merged
merged 1 commit into from
Feb 17, 2020

Conversation

jellyedwards
Copy link
Contributor

This change is aimed to solve #10664 - only upload the texture to the GC if the texture has changed as it's an expensive operation and leads to reduced performance.

@jellyedwards jellyedwards requested review from jahow and removed request for jahow February 17, 2020 09:51
@jellyedwards
Copy link
Contributor Author

Sorry, just noticed a test failed:

Chrome 80.0.3987 (Windows 10.0.0) HTML Image loading handles error event FAILED
Error: Uncaught Error: expected false to equal true
Chrome 80.0.3987 (Windows 10.0.0) HTML Image loading handles error event FAILED
Error: Uncaught Error: expected false to equal true

I'll look into it...

@fredj
Copy link
Member

fredj commented Feb 17, 2020

@jellyedwards
Copy link
Contributor Author

@fredj yes that looks like it, I'll just try it on the original to double-check it wasn't there before my fix for the issue...

@jellyedwards
Copy link
Contributor Author

@fredj yeah that test failed before my changes too. This is my first time contributing, but I could look into why that test is failing. Just to speed things up how do you run a single test?

@fredj
Copy link
Member

fredj commented Feb 17, 2020

This test is only failing on Windows and it's related to the timeout value; I've changed the value from 200 to 500ms and it is passing:

diff --git a/test/spec/ol/image.test.js b/test/spec/ol/image.test.js
index bf6e19ef0..b6f75abf4 100644
--- a/test/spec/ol/image.test.js
+++ b/test/spec/ol/image.test.js
@@ -40,7 +40,7 @@ describe('HTML Image loading', function() {
       expect(handleLoad.called).to.be(false);
       expect(handleError.called).to.be(true);
       done();
-    }, 200);
+    }, 500);
   });

   it('handles cancelation', function(done) {

Copy link
Contributor

@jahow jahow left a comment

Choose a reason for hiding this comment

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

Thanks @jellyedwards!

Please let me know if you want to include a fix for the failing test on Windows before merging.

@fredj
Copy link
Member

fredj commented Feb 17, 2020

@jahow I'm creating a PR for the failing test. This one can be merged.

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.

None yet

3 participants