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

Timeline autoPan implemented #897

Closed
wants to merge 2 commits into from

Conversation

raviagrwl420
Copy link
Contributor

This pull request attempts to solve #773.
However some guidance regarding the fixes is required.

}

if (this._autoPan !== 0) {
if ((xPos > 3 * width / 8) && (xPos < 5 * width / 8 )) {
Copy link
Contributor

Choose a reason for hiding this comment

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

So, it turns out this is "shaky" because xPos here is calculated based on _scrubJulian after it was already advanced by the clock. What you need is the xPos from the previous frame, which happens to be available as this._lastXPos already. This tells you where the scrubber was before time advanced, so you know if it used to be in the double-speed zone or not. You only need _lastXPos here on line 514, not on the lines above.

@emackey
Copy link
Contributor

emackey commented Jun 27, 2013

This is a cool feature. I found another small bug: Load CesiumViewer/index.html?source=Gallery/simple.czml, and use the right mouse button (drag it to the right on the timeline) to zoom in, the autopan gets confused when time "loops" from end back to start time. I think the correct behavior would be to jump the timeline back to the start and shut off autopan.

@raviagrwl420
Copy link
Contributor Author

I think something went wrong.
Can somebody please explain me!

@mramato
Copy link
Contributor

mramato commented Jul 10, 2013

Everything looks okay to me, what do you think is wrong?

@emackey
Copy link
Contributor

emackey commented Jul 10, 2013

It's a rebased merge from main, so there are copies of changelists from other folks. Fixing it is tricky, but goes something like this. First, make a backup of the one file you changed. Then:

git checkout master

Stop and make sure "master" is up to date with the latest upstream copy of master, then continue:

git branch -D TimelineScrubber
git checkout -b TimelineScrubber
git cherry-pick 30f6ab5
git cherry-pick 971a4b1

Stop again and make sure the newly created TimelineScrubber branch has all your changes from the old branch. When you're satisfied it does, force-push it to GitHub, to overwrite the old branch.

git push -f origin TimelineScrubber

This pull request should update after you do this, and the extra commits should disappear. I'm not sure what exactly causes this problem, but I'm pretty sure it has something to do with our advice for everyone to turn on automatic rebase for all git pulls.

@shunter
Copy link
Contributor

shunter commented Jul 10, 2013

Actually, the extra commits don't look like they were rebased. The SHA1s match the commits in Cesium master. I'm not sure why GitHub is showing them as part of the pull request when git log master..HEAD does not.

@raviagrwl420
Copy link
Contributor Author

Thanks @emackey ,
I got a little panicked due to the Travis CI thing which started as soon as I did the commit.
Following your instructions, I found it better to use a perfectly clean copy of master.

Also regarding the bug related to CesiumViewer/index.html?source=Gallery/simple.czml what would be the best way to know when the time loops back to the start time. Since the looping is enforced by the tick function in Clock.js, do I need to make the changes here?

@emackey
Copy link
Contributor

emackey commented Jul 10, 2013

No, just check if the current time is on the opposite side of the screen from the autopan direction.

@mramato
Copy link
Contributor

mramato commented Jul 15, 2013

@macoda is this ready for another review? Whenever someone requests changes during a review, it's good practice to post a message after you have made them, this way we know to look at it again.

@raviagrwl420
Copy link
Contributor Author

@mramato
I'm really sorry! I usually do post a message but sometimes thought it as redundant.
Thanks for that, I'll keep it in mind.

Yes this is ready for another review. I have resolved @emackey 's concerns in the previous commit.

@emackey
Copy link
Contributor

emackey commented Jul 16, 2013

@macoda yes GitHub doesn't send automatic notifications of each commit, because sometimes it takes multiple commits to get to a state where you want more feedback. So, writing a comment asking for additional review is the recommended way to go.

I like this branch, but the performance is hurting, and its my own fault. I wrote the timeline a couple years ago before I knew much about animation on web pages. The tic marks are all DOM elements that get destroyed, and then parsed from a string to be re-created every time the zoom level changes, which is every animation frame when auto-pan is engaged. I know now that it's about the worst way, performance-wise, to animate it.

There are better ways of course, but let's put this branch aside and focus on the new flat-style widgets you've got going in your other two branches. We'll hold onto this autopan code for later, if there's extra time at the end or after GSoC even, we could circle back to this and design re-usable tics or convert this to SVG or Canvas to get some different performance numbers going.

@raviagrwl420
Copy link
Contributor Author

@emackey Seems good to me. We can definitely come back to this later.

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 15, 2013

This has been opened for 4 months, what is the plan?

@mramato
Copy link
Contributor

mramato commented Oct 15, 2013

We want this change, but we can't take it yet because it's a performance killer (due to the current implementation of the timeline). I was thinking about parking it on a branch, but it's easier to just have it open for now.

@pjcozzi
Copy link
Contributor

pjcozzi commented Apr 8, 2014

Is this still waiting on a timeline refactor? If so, let's just put it on a branch and reference it from a roadmap. No need to have open pull requests just as reminders.

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 14, 2014

Should we park this in a branch?

@emackey
Copy link
Contributor

emackey commented Dec 14, 2014

Yeah it seems like the timeline re-write is still not a high priority, at least for the project I'm on. I would like to get this going again someday.

@emackey emackey closed this Dec 14, 2014
@emackey
Copy link
Contributor

emackey commented Dec 14, 2014

This lives on in macoda's fork and the emackey fork as "TimelineScrubber".

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.

5 participants