OpenGeoscience / geoweb

Open source geoscience tools and applications
8 stars 3 forks source link

Animation runs only once #154

Closed aashish24 closed 11 years ago

aashish24 commented 11 years ago

Drag and drop clt. Run the animations, remove the clt, drag and drop clt again. Animation does not work.

benbu commented 11 years ago

It appears that the animation workflows are executing and returning new data, but it's just not updating the display. Still looking for the culprit.

benbu commented 11 years ago

If you use 'stop', it works, if you use 'pause', it doesn't. This section of code also looks suspicious, and I think it's what's preventing the view from updating. It's from map.js in function predraw on line 576.

    if (m_featureCollection.getMTime() >
        m_lastPrepareToRenderingTime.getMTime()) {

      // Remove expired features from the renderer
      for (layerName in m_layers) {
        m_renderer.removeActors(
          m_featureCollection.expiredFeatures(layerName));

        // Add new actors (Will do sorting by bin and then material later)
        m_renderer.addActors(
          m_featureCollection.newFeatures(layerName));
      }

      m_featureCollection.resetAll();
      m_lastPrepareToRenderingTime.modified();
    }

It appears that if the animation was paused, the layer removed, then re-added, the m_featureCollection.getMTime() call is less than the m_lastPrepareToRenderingTime.getMTime() call, so it never pre-renders.

Also note that you can get the animations to work again if you press some combination of stop,pause,and play enough times.

Any ideas?

aashish24 commented 11 years ago

The pause is broken. But the original bug is different. If remove the layer and add it back again, it does not animate.

benbu commented 11 years ago

This is what I'm experiencing:

Add clt, remove it, add it again, animate. Works. reload Add clt, animate, 'stop', remove it, add it again, animate. Works. reload Add clt, animate, 'pause', remove it, add it again, animate. Doesn't work!

After the last attempt using 'pause', I notice that the pause button is still selected as soon as I re-add the clt layer. Also, I can get the animation to work from here if I press some random combination play, pause, stop enough times.

So to me, it appears to be a 'pause' issue.

aashish24 commented 11 years ago

Ah... I see. Thanks for the clarification. When we remove it, it should start from scratch. I wonder why the previous state matters. @cjh1 ?

aashish24 commented 11 years ago

@benbu I requested @cjh1 to look at this issue because I suspect its the UI code causing the issue.

cjh1 commented 11 years ago

154-fix-animation

benbu commented 11 years ago

Looks good to me, the issue was fixed.

There are some jslint infractions on the two edited files however ;)

~/src/climatepipes/source$ jslint web/lib/geo/map.js 

web/lib/geo/map.js
 #1 Unexpected 'this'.
    this.animateInternal = function() { // Line 729, Pos 3
 #2 Stopping.  (73% scanned).
     // Line 729, Pos 3
~/src/climatepipes/source$ jslint web/lib/ui/gis.js 

web/lib/ui/gis.js
 #1 Combine this with the previous 'var' statement.
    var ensureTimeInfo = function(layerIds, onDone) { // Line 179, Pos 7
 #2 Expected ';' and instead saw 'var'.
    }) // Line 187, Pos 7
 #3 Combine this with the previous 'var' statement.
    var numberOfRequests = datasets.length; // Line 188, Pos 9
 #4 Expected '===' and instead saw '=='.
    if (numberOfRequests == 0) // Line 191, Pos 26
 #5 Expected '{' and instead saw 'onDone'.
    onDone(layerIds); // Line 192, Pos 9
 #6 Expected '===' and instead saw '=='.
    if (numberOfRequests == 0) // Line 201, Pos 32
 #7 Expected '{' and instead saw 'onDone'.
    onDone(layerIds); // Line 202, Pos 13
 #8 Combine this with the previous 'var' statement.
    var hideTimeStepDisplay = function() { // Line 207, Pos 7
 #9 Unexpected '$'.
    $('#play', animationControls).click(function() { // Line 213, Pos 3
#10 Stopping.  (24% scanned).
     // Line 213, Pos 3
cjh1 commented 11 years ago

I will take a look at them ...

On Fri, Sep 20, 2013 at 3:00 PM, benbu notifications@github.com wrote:

Looks good to me, the issue was fixed.

There are some jslint infractions on the two edited files however ;)

~/src/climatepipes/source$ jslint web/lib/geo/map.js

web/lib/geo/map.js

1 Unexpected 'this'.

this.animateInternal = function() { // Line 729, Pos 3

2 Stopping. (73% scanned).

 // Line 729, Pos 3

~/src/climatepipes/source$ jslint web/lib/ui/gis.js

web/lib/ui/gis.js

1 Combine this with the previous 'var' statement.

var ensureTimeInfo = function(layerIds, onDone) { // Line 179, Pos 7

2 Expected ';' and instead saw 'var'.

}) // Line 187, Pos 7

3 Combine this with the previous 'var' statement.

var numberOfRequests = datasets.length; // Line 188, Pos 9

4 Expected '===' and instead saw '=='.

if (numberOfRequests == 0) // Line 191, Pos 26

5 Expected '{' and instead saw 'onDone'.

onDone(layerIds); // Line 192, Pos 9

6 Expected '===' and instead saw '=='.

if (numberOfRequests == 0) // Line 201, Pos 32

7 Expected '{' and instead saw 'onDone'.

onDone(layerIds); // Line 202, Pos 13

8 Combine this with the previous 'var' statement.

var hideTimeStepDisplay = function() { // Line 207, Pos 7

9 Unexpected '$'.

$('#play', animationControls).click(function() { // Line 213, Pos 3

10 Stopping. (24% scanned).

 // Line 213, Pos 3

— Reply to this email directly or view it on GitHubhttps://github.com/OpenGeoscience/geoweb/issues/154#issuecomment-24833635 .

cjh1 commented 11 years ago

Fixed them, although I think the Combine this with the previous 'var' statement. one makes things harder to read ...

benbu commented 11 years ago

Approved.

aashish24 commented 11 years ago

@cjh1 Can you push 154-fix-animation upstream as well? You just merged them to next.

cjh1 commented 11 years ago

It is:

git push origin 154-fix-animation Everything up-to-date

What am I missing?

On Mon, Sep 23, 2013 at 10:33 AM, Aashish Chaudhary < notifications@github.com> wrote:

@cjh1 https://github.com/cjh1 Can you push 154-fix-animation upstream as well?

— Reply to this email directly or view it on GitHubhttps://github.com/OpenGeoscience/geoweb/issues/154#issuecomment-24924123 .

aashish24 commented 11 years ago

what's your origin is? (git remote -v)

benbu commented 11 years ago

I'm seeing this branch on the kitware origin

git remote -v 
github  https://github.com/OpenGeoscience/geoweb.git (fetch)
github  https://github.com/OpenGeoscience/geoweb.git (push)
origin  git://public.kitware.com/OpenGeoscience/geoweb.git (fetch)
origin  git@public.kitware.com:OpenGeoscience/geoweb (push)
git branch -r 
...
origin/154-fix-animation
...
aashish24 commented 11 years ago

Thanks, I see it now as well. Also, I pushed 154-fix-animation-2. @cjh1 @benbu Could you please approve.

Couple of things: private methods / functions shouldn't be this.foo = function () {}. Also, Comments for 1-2 methods were missing (minor fixes).

cjh1 commented 11 years ago

Approved

On Tue, Sep 24, 2013 at 10:12 AM, Aashish Chaudhary < notifications@github.com> wrote:

Thanks, I see it now as well. Also, I pushed 154-fix-animation-2. @cjh1https://github.com/cjh1 @benbu https://github.com/benbu Could you please approve.

Couple of things: private methods / functions shouldn't be this.foo = function () {}. Also, Comments for 1-2 methods were missing (minor fixes).

  • Aashish

— Reply to this email directly or view it on GitHubhttps://github.com/OpenGeoscience/geoweb/issues/154#issuecomment-25007155 .

aashish24 commented 11 years ago

Thanks.. merged