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

Animated images for traces #2204

Merged
merged 9 commits into from Jun 10, 2019
Merged

Conversation

@gravitystorm
Copy link
Collaborator

@gravitystorm gravitystorm commented Apr 10, 2019

Here's a pull request based on @mmd-osm's work with a few minor changes from me. It produces animated gifs that look identical to those generated by the external importer.

This PR contains two notable caveats:

  • We uncovered a bug in libgd itself. This PR works around the bug, and I wouldn't propose waiting for that bug to be fixed and the fix to work its way into distribution packages since that could take a while.
  • We are using a forked version of the gd2-ffij gem, until the animation code is available in the upstream gem. I'm fine with that from a bundler point of view, but of course there's the risk that the animation code is either not upstreamed, or upstream ends up with a different implementation.
mmd-osm and others added 2 commits Apr 3, 2019
This works around an issue in libgd2 library which would otherwise
cause segfaults due to zero sized images
Gemfile Outdated
@@ -117,7 +117,7 @@ gem "canonical-rails"
gem "logstasher"

# Used to generate images for traces
gem "gd2-ffij", :git => 'git@github.com:mmd-osm/gd2-ffij.git', :branch => 'animated_gif'
gem "gd2-ffij", :git => "git@github.com:mmd-osm/gd2-ffij.git", :branch => "animated_gif"
Copy link
Contributor

@mmd-osm mmd-osm Apr 10, 2019

The git protocol link causes some issues with TravisCI. Maybe try https://github.com/mmd-osm/gd2-ffij.git instead?

@mmd-osm
Copy link
Contributor

@mmd-osm mmd-osm commented Apr 12, 2019

Upstream pull request for gd2-ffij: dark-panda/gd2-ffij#19

@tomhughes
Copy link
Member

@tomhughes tomhughes commented Apr 14, 2019

We're not able to use git deployed gems in our production environment so we can't merge this as is anyway - we would need to publish a forked version to rubygems in order to go this route.

@gravitystorm
Copy link
Collaborator Author

@gravitystorm gravitystorm commented Apr 25, 2019

We're not able to use git deployed gems in our production environment

Is this by policy, or for a technical reason? If it's just a technical reason that needs fixing, then I can do the relevant work on the chef cookbook.

@tomhughes
Copy link
Member

@tomhughes tomhughes commented Apr 25, 2019

I honestly can't remember, I just know it didn't work.

To be honest given we install globally I'm not even sure what happens if you try and do a git based install, but I'm pretty sure it won't be very nice.

@mmd-osm
Copy link
Contributor

@mmd-osm mmd-osm commented May 7, 2019

Awesome, we have some progress in dark-panda/gd2-ffij#18

@tomhughes , @gravitystorm : could you take a look if this makes sense to you? Thanks!

@dark-panda
Copy link

@dark-panda dark-panda commented May 13, 2019

If this API on my end looks good, I'll wrap this up and do a release quickly for you. Just give me the word and we can wrap this one up.

@tomhughes
Copy link
Member

@tomhughes tomhughes commented May 13, 2019

@mmd-osm are you happy with it? have you got a version of our code that works with it?

@mmd-osm
Copy link
Contributor

@mmd-osm mmd-osm commented May 13, 2019

I was hoping for @gravitystorm to take a look, too. From my side we’re good, one of my code snippets made into the test case collection, and that looks fine.

@tomhughes
Copy link
Member

@tomhughes tomhughes commented May 13, 2019

I've tested a modified version of this branch with @dark-panda's code and it all seems to work fine so yes @dark-panda please do go ahead and make a new release.

@gravitystorm
Copy link
Collaborator Author

@gravitystorm gravitystorm commented May 15, 2019

I was hoping for @gravitystorm to take a look, too.

Sorry - I've been busy for a few weeks (mostly holidays). I think the proposed new interface by @dark-panda looks great, so I look forward to the next release.

@Firefishy
Copy link
Member

@Firefishy Firefishy commented Jun 10, 2019

Hope you don't mind. I resolved the merge conflicts.

@mmd-osm
Copy link
Contributor

@mmd-osm mmd-osm commented Jun 10, 2019

0.4.0 is out since a few minutes! https://rubygems.org/gems/gd2-ffij/versions/0.4.0

@tomhughes tomhughes merged commit c17cd3a into openstreetmap:master Jun 10, 2019
@mmd-osm
Copy link
Contributor

@mmd-osm mmd-osm commented Jun 11, 2019

I think we’re only displaying 9 instead of 10 frames after moving to the 0.4.0 API. If you look at the example dark-panda/gd2-ffij#18 (comment) it’s clear that we need to add frame 1 twice, which we fail to do atm.

It’s probably better to add the first frame right before entering the loop, otherwise the animation doesn't start with the beginning of the GPX track, which is confusing.

      image = GD2::AnimatedGif.new
      image.add(frames[0])                 " << new line to be added
      frames.each do |frame|
        image.add(frame, :delay => delay)
      end
      image.end

@mmd-osm
Copy link
Contributor

@mmd-osm mmd-osm commented Jun 11, 2019

@tomhughes : I feels a bit like overkill to create a new pull request for this one liner. Could you please take over this issue?
See https://www.openstreetmap.org/user/IDVGROUP5/traces/3013715 (current) vs. https://www.openstreetmap.org/user/rentinn/traces/3012941 (how it should be)

@mmd-osm
Copy link
Contributor

@mmd-osm mmd-osm commented Jun 11, 2019

I think we have one more issue with the points_per_frame calculation, which is visible in the following trace: https://www.openstreetmap.org/user/Waclaw1/traces/3014390
Parts of the trace are newer shown in black with thickness 3.

3014390

The issue here is probably that the current points_per_frame logic only applies integer division, so that some points towards the end of a trace might be omitted. Could we change this to something like:

      points_per_frame = (num_points / nframes.to_f).ceil

Result looks better then:

136

@gravitystorm
Copy link
Collaborator Author

@gravitystorm gravitystorm commented Jun 12, 2019

I feels a bit like overkill to create a new pull request for this one liner.

It's always worthwhile creating a pull request - firstly it gives you credit, and secondly it removes any ambiguity over exactly which line you mean. There's also another practical reason:

Could you please take over this issue?

Tom's a busy guy, so it's worth sharing the development load around rather than asking him to take on more tasks himself. I'm happy to review and merge pull requests, as is Tom. I prefer to have a review of the work I've created myself. So in general there are three options available:

  • Tom does the work himself, committing directly to master
  • I make a PR, and Tom reviews it before merging to master
  • Someone else makes a PR, then either me or Tom reviews it before merging to master

Only the final option can avoid more work for Tom! So it's best to create PRs whenever possible.

@tomhughes
Copy link
Member

@tomhughes tomhughes commented Jun 12, 2019

I try and do PRs for my stuff where it's non-urgent as well especially if it's larger or controversial in any sort of way...

Which reminds me - any thoughts people have on #2209 would be appreciated ;-)

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

Successfully merging this pull request may close these issues.

None yet

5 participants