CartoDB / torque

Temporal mapping for CARTO
http://cartodb.github.com/torque/
BSD 4-Clause "Original" or "Old" License
397 stars 131 forks source link

Rendering problem with Firefox #280

Closed rochoa closed 7 years ago

rochoa commented 7 years ago

This replaces https://github.com/CartoDB/cartodb/issues/11466.

Context

Torque visualizations don't work well in Firefox v51.

Tiles are rendered, but for some of them are not 'cleared' and the result is similar to having the 'cumulative' option enabled.

This happens only in Firefox, and not on every tile, as some of them work as expected.

Steps to Reproduce

  1. Make sure you're using Firefox v51
  2. Open this map https://team.carto.com/u/ernestomb/builder/cd37d292-ec93-11e6-9e93-0e3ebc282e83/embed
  3. Check that markers are not cleared and they start accumulating, but only on some of the tiles

Current Result

Torque maps aren't working well on Firefox

screen shot 2017-02-06 at 18 51 37

Expected result

Markers should be cleared from the map

Browser and version

Firefox v51

rochoa commented 7 years ago

This seems related to the old trick we use to clear the canvas, see https://github.com/CartoDB/torque/blob/d218c2da24709b1d730e43673379035e26546935/lib/torque/renderer/point.js#L92.

Doing a quick test using clearRect seems to fix the problem in Firefox, however, that might not work for other browsers (e.g. IE).

I tested with:

diff --git a/lib/torque/renderer/point.js b/lib/torque/renderer/point.js
index 5dce63a..609086c 100644
--- a/lib/torque/renderer/point.js
+++ b/lib/torque/renderer/point.js
@@ -89,7 +89,7 @@ var CartoDatasource = require('./datasource');
         var color = this._Map['-torque-clear-color']
         // shortcut for the default value
         if (color  === "rgba(255, 255, 255, 0)" || !color) {
-          this._canvas.width = this._canvas.width;
+          this._ctx.clearRect(0, 0, canvas.width, canvas.height);
         } else {
           var ctx = this._ctx;
           ctx.setTransform(1, 0, 0, 1, 0, 0);

As we were already using the width trick, I guess it's not a problem to ignore previous transformations. Otherwise, it's important to consider them, see http://stackoverflow.com/a/6722031.

javisantana commented 7 years ago

clearRect is the right way but we used that trick because it was faster (probably no longer true)

donflopez commented 7 years ago

Thanks, @rochoa! That was easy with your help!

As your link to StackOverflow says, we have to reset the transform to have a correct clearRect over the whole canvas.

There is not an important performance change.

I'll make a PR :)

donflopez commented 7 years ago

PR here

dgaubert commented 7 years ago

Do you some guidance to do acceptance check here?

donflopez commented 7 years ago

The acceptance is the same that the issue describes

dgaubert commented 7 years ago

I'm a newbie in Torque's world. My guess is that I'll need some setup in my local env to link the hotfix-branch to our stack and test it. Do you know what I'll need?

donflopez commented 7 years ago

Ouch, sorry, the prerequisites:

dgaubert commented 7 years ago

screen shot 2017-02-08 at 15 29 45

LGTM 🇨🇭

donflopez commented 7 years ago

Cool!! Thanks @dgaubert!!

rochoa commented 7 years ago

Have you tested this in other browsers? Internet Explorer?

donflopez commented 7 years ago

Me not, are we supporting IE @rochoa?

dgaubert commented 7 years ago

I've tested only for: firefox and chrome

rochoa commented 7 years ago

We do. We need to test IE9+ and Edge. Also, don't forget Safari.

dgaubert commented 7 years ago

Safari 10.0.1 (11602.2.14.0.7): 🆗

donflopez commented 7 years ago

Ups, I didn't know it! Is there any windows laptop to try?

rochoa commented 7 years ago

What's the status on the IE testing?

For IE you can use https://developer.microsoft.com/en-us/microsoft-edge/tools/vms/.

donflopez commented 7 years ago

Trying!

donflopez commented 7 years ago

It is working as expected! In Edge, downloading IE 9:)

screen shot 2017-02-13 at 12 08 06

rochoa commented 7 years ago

What about older versions? I'm worried about IE9, see http://stackoverflow.com/a/4085780.

donflopez commented 7 years ago

It doesn't work on IE9, but It didn't work before this changes, there are other problems @rochoa

donflopez commented 7 years ago

I've tested on IE10 and it works

rochoa commented 7 years ago

Then I would :shipit:.

ernesmb commented 7 years ago

Will this fix also work for maps created with cartodb.js?

donflopez commented 7 years ago

I'm not sure about the differences, sorry... but I'd say yes. Maybe @rochoa or @alonsogarciapablo can say more about this :)

rochoa commented 7 years ago

I won't if we don't release new versions of cartodb.js. @ernesmb Can you be more specific about what version you are talking about?

ernesmb commented 7 years ago

yes sorry, clients are asking about cartodb.js v3.12.11

let me ask them again just to be 100% sure

sorry for the noise

rochoa commented 7 years ago

OK, after talking to @ernesmb about this offline, we would release a new version of cartodb.js v3.15.x to address the issue.

donflopez commented 7 years ago

It is deployed in production, closing

rochoa commented 7 years ago

@ernesmb, @donflopez just published a new version of cartodb.js including the fixes.