oakmac / chessboardjs

JavaScript chessboard
https://chessboardjs.com
MIT License
2.01k stars 408 forks source link

Pieces flickers on update #52

Open sesse opened 10 years ago

sesse commented 10 years ago

Hi,

When I do board.position(fen), it would seem the pieces are somehow redrawn; at least they flicker ever so shortly during the move somehow. Especially for the pieces that stand still, maybe this could avoided?

I don't think this has anything to do with my code; it happens on e.g. http://chessboardjs.com/examples#3001 when I switch positions.

I'm using Chrome 31.0.1650.57 on Linux.

bs commented 10 years ago

I'm experiencing the same, but haven't had a chance to dive into it yet.

Interestingly, the examples on http://chessboardjs.com seem to work fine, but my implementation flickers. The only difference is that I have specified paths to the assets. Going to try to find some time to research this further within the next week.

Doesn't flicker in Firefox, only Chrome and Safari.

oakmac commented 10 years ago

@sesse - can you make a video of this happening?

sesse commented 10 years ago

Sure; see http://storage.sesse.net/chessboardjs-flicker.avi. You can see it flickers a few times there (although not every time).

bs commented 10 years ago

Did a big of debugging.

The really odd bit is that I can reproduce it locally, but don't see it at all on the examples on chessboardjs.com. Just tried running it through a Node Express server in case Chrome's file:// was at fault, but still flickers.

Here's my small test code: https://github.com/bs/jschesstest And here's a video of it in action: https://vimeo.com/80140179

@oakmac, can you try running that code locally and see if you can reproduce?

I've chased it down to the drawPositionInstant method when called from doAnimations.

Zolmeister commented 10 years ago

I may have found a solution, see #55

bs commented 10 years ago

@Zolmeister Doesn't fix.

bs commented 10 years ago

This seems to be a Node/Express issue for me. Running my test via python -m SimpleHTTPServer is silky smooth.

sesse commented 10 years ago

My guess is that the entire thing with the web server (be it Node.js, local serving or whatever) is pretty much incidental; there are no HTTP requests going on during a move, even though you could maybe imagine something odd with different HTTP headers. A JavaScript library shouldn't be flicker no matter what server originally served its assets anyway, though.

Zolmeister commented 10 years ago

I was most definitely recording an HTTP request during an animation, what may be happening is the 'Expires' cache header may not be set for the basic Express server.

Here on line 654:

function buildPiece(piece, hidden, id) {
  var html = '<img src="' + buildPieceImgSrc(piece) + '" ';
  if (id && typeof id === 'string') {
    html += 'id="' + id + '" ';
  }
  html += 'alt="" ' +
  'class="' + CSS.piece + '" ' +
  'data-piece="' + piece + '" ' +
  'style="width: ' + SQUARE_SIZE + 'px;' +
  'height: ' + SQUARE_SIZE + 'px;';
  if (hidden === true) {
    html += 'display:none;';
  }
  html += '" />';

  return html;
}

Is being called here, and here

bs commented 10 years ago

Setting the Expires header fixed it in Express for me.

Still, is proper request caching needed here? The buildPiece method is being called once for every piece on the board whenever a move occurs. Is there a reason each HTML chunk isn't build once and cached?

oakmac commented 10 years ago

The webserver sending appropriate expires headers and returning HTTP 304 Not Modified is probably the solution here (at least for now).

I might change the way this works in the future.

oakmac commented 10 years ago

related: http://stackoverflow.com/questions/20277073/animation-blinks-without-base-href-only-for-google-chrome

Zolmeister commented 10 years ago

Even with HTTP 304 header you still get flickering (which is why I implemented #55).

sesse commented 10 years ago

Hi,

The solution in #55 still flickers for me (judging from http://queens.zolmeister.com/), just as much as the normal chessboard.js does. And there is not a single HTTP request going on, so really, all this talk about Expires and 304 is not the full story.

Zolmeister commented 10 years ago

Indeed, but I think #55 highlights the root of the problem, which is that the way animations are written, a new piece is generated and added (i.e. a new element) which seems to me like the most likely culprit.

sesse commented 10 years ago

On Fri, Nov 29, 2013 at 02:01:04AM -0800, Zoli Kahan wrote:

Indeed, but I think #55 highlights the root of the problem, which is that the way animations are written, a new piece is generated and added (i.e. a new element) which seems to me like the most likely culprit.

That sounds much more likely, yes. If possible, elements should be reused (I don't know the best strategy for introducing new pieces on the board, but that should be much more rare).

/* Steinar */

Homepage: http://www.sesse.net/

ingararntzen commented 10 years ago

it flickers in chrome, but not in Firefox as far as I can see.

(I haven't implemented #55)

I'd be keen to have an update that works in chrome too :)

Cheers,

Ingar Arntzen

ingararntzen commented 10 years ago

Hi - I have fixed the problem by reprogramming how the visuals are updated.

The idea is to avoid flushing the entire board on every move/position update. Instead I calculate the delta between CURRENT_POSITION and new POSITION, and then touch only those squares that need to be touched on every position update.

This solves the flickering problem for Chrome it seems - and the solution works both for immediate updates as well as animated.

Here are some code snippets

1) let setCurrentPosition return old position

function setCurrentPosition(position) {
    ...
    return oldPos;
}

2) In widget.position (at the very end) make sure drawPositionInstant is called with the old position

widget.position = function(position, useAnimation) {
  ...
    var old_pos = setCurrentPosition(position);
    drawPositionInstant(old_pos);
  }
};

3) In doAnimations - make sure drawPositionInstant is called with old position too

// execute an array of animations
function doAnimations(a, oldPos, newPos) {
  ...
  function onFinish() {
    ...
    drawPositionInstant(oldPos);
    ...
}

4) Add some helper functions for calculating position delta

    /*
      return keys found in obj_a but not in obj_b
     */
    var find_unique = function(obj_a, obj_b) {
    var res = [];
    for (var key in obj_a) {
        if (!obj_a.hasOwnProperty(key)) {continue;}
        if (!obj_b.hasOwnProperty(key)) {
        res.push(key);
        }
    }
    return res;
    };

    /*
      return keys found in both obj_a and obj_b
     */
    var find_common = function (obj_a, obj_b) {
    var res = [];
    for (var key in obj_a) {
        if (!obj_a.hasOwnProperty(key)) {continue;}
        if (obj_b.hasOwnProperty(key)) {
        res.push(key);
        }
    }
    return res;
     };

5) Update drawPositionInstant to take old position into consideration

function drawPositionInstant(old_pos) {
    var new_pos = CURRENT_POSITION;
    if (old_pos !== undefined) {

    /* optimization - update visuals based on calculating delta between
       old position and new position

       sort squares (position keys) into nonoverlapping sets,
       - removed : squared where the piece need to be removed
       - added : square where a piece need to be added
       - replaced: square where piece must be replaced 

       Note: we actually end up treating added and replaced
       equally, presumably due to a weakness in cleaning up after
       animations. So, in hindsight we would only need two sets
       - removed
       - updated
    */
    var clear_list = find_unique(old_pos, new_pos);
    var add_list = find_unique(new_pos, old_pos);
    var common = find_common(old_pos, new_pos); 
    var replace_list = [];
    for (var i=0; i<common.length; i++) {
        var key = common[i];
        if (old_pos[key] !== new_pos[key]) {
        replace_list.push(key);
        }
    }   

    /* 
       possible optimization 
       make a {} linking all 64 squares to their respective jQuery elements
       so that we don't have to search the DOM on every update
    */

    // clear pieces
    for (var i in clear_list) {
        var square = clear_list[i];
        $('#' + SQUARE_ELS_IDS[square]).find('.' + CSS.piece).remove();
    }
    // replace pieces
    for (var i in replace_list) {
        var square = replace_list[i];
        var $elem = $('#' + SQUARE_ELS_IDS[square]);
        $elem.find('.' + CSS.piece).remove();
        $elem.append(buildPiece(CURRENT_POSITION[square]));
    }
    // add pieces
    for (var i in add_list) {
        var square = add_list[i];
        var $elem = $('#' + SQUARE_ELS_IDS[square]);
        // note - need to remove here, presumably because
        // animation may be going on
        $elem.find('.' + CSS.piece).remove();
        $elem.append(buildPiece(CURRENT_POSITION[square]));
    }
    }
    else {
    // clear the board
    boardEl.find('.' + CSS.piece).remove();
    // add the pieces
    for (var i in CURRENT_POSITION) {
        if (CURRENT_POSITION.hasOwnProperty(i) !== true) { 
        continue;
        }
        $('#' + SQUARE_ELS_IDS[i]).append(buildPiece(CURRENT_POSITION[i]));
    }
    }
}

I think that's it.

Ingar

jurepolutnik commented 10 years ago

55 Resolves the problem on Mac OSX / Chrome.

cacheImage() must be called after ChessBoard.init().

jurepolutnik commented 10 years ago

In addition to previous answer... In buildPiece() new tag is created and than appended. This causes browser to make a new request to server. To prevent this, configure Expires meta-tag in header accordingly.

shaktisd commented 10 years ago

Resolved the constant refresh issue by adding ,"Cache-Control" : "max-age=604800" to headers.

knk9 commented 6 years ago

Future reference for anybody trying to solve this issue using Spring: sping-security is overriding the headers. Add this to your WebMvcConfigurerAdapter:

 @Override
    public void addResourceHandlers(ResourceHandlerRegistry registry) {
        registry.addResourceHandler("/img/**")
                .addResourceLocations("classpath:/static/img/")
                .setCachePeriod(12);
    }
knaveenchand commented 4 years ago

starting the node server resolved this problem for me.

codejunkiepro commented 3 years ago

Thanks @shaktisd 👍👍

frederikme commented 2 years ago

Still having this problem sadly, any updates?

frederikme commented 2 years ago

Firefox & Chrome were working fine, the issue only occurred for Safari on Mac and IOS. I managed to solve the issue for myself by changing a setting on the server side using Flask, Python.

app = Flask(__name__)
app.send_file_max_age_default = 300
sesse commented 2 years ago

In a great fit of karma, I've seemingly joined the Chrome team responsible for this, and discovered that there is now a bug that seems possibly related: https://crbug.com/961600

rorycochrane commented 2 years ago

@frederikme I'm having the same issue, just for Safari on Mac and iOS. Unfortunately the solution you posted doesn't seem to work for me. Did you make any other changes besides setting send_file_max_age_default to 300?