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

Ensure images always fade in completely, fix broken "raster-fade-duration" property #3532

Merged
merged 3 commits into from
Nov 3, 2016

Conversation

lucaswoj
Copy link
Contributor

@lucaswoj lucaswoj commented Nov 3, 2016

As explained in #3526 , Mapbox GL JS does not properly implement raster-fade-duration. This PR fully removes support for "raster-fade-duration" and thereby ensures that images always fade in completely.

cc @jfirebaugh @mollymerp @mourner @mapsam

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality we do not have any infrastructure to test animations
  • document any changes to public APIs
  • manually test the debug page

Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

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

A ton of raster render tests are now failing. Also, Style sets rasterFadeDuration in _recalculate — could there be a better place for the constant?

@@ -156,6 +156,8 @@ class ImageSource extends Evented {
gl.texParameteri(gl.TEXTURE_2D, gl.TEXTURE_MIN_FILTER, gl.LINEAR);
gl.texParameteri(gl.TEXTURE_2D, gl.TEXTURE_MAG_FILTER, gl.LINEAR);
gl.texImage2D(gl.TEXTURE_2D, 0, gl.RGBA, gl.RGBA, gl.UNSIGNED_BYTE, image);
this.map.animationLoop.set(this.tile.timeAdded + this.map.style.rasterFadeDuration - Date.now());
Copy link
Member

Choose a reason for hiding this comment

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

Why did animation loop work for raster fades before adding this line? Do we set the loop in several places for this?

@lucaswoj
Copy link
Contributor Author

lucaswoj commented Nov 3, 2016

Because of the # of failing tests and the additional workload of safely deprecating this feature, I have decided to change direction and fix our support for raster-fade-duration. We can continue to discuss deprecation separately.

@lucaswoj lucaswoj changed the title Ensure images always fade in completely, remove broken "raster-fade-duration" property Ensure images always fade in completely, fix broken "raster-fade-duration" property Nov 3, 2016
Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

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

Looks so much cleaner now! Merge on green.

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

Successfully merging this pull request may close these issues.

2 participants