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.

Sign up for GitHub

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

Jump to bottom

Imagemagick image writer #1304

Merged
merged 15 commits into from
Jan 31, 2018
Merged

Imagemagick image writer #1304

merged 15 commits into from
Jan 31, 2018

Conversation

dcoeurjo
Copy link
Member

@dcoeurjo dcoeurjo commented Dec 30, 2017 •
edited
Loading

PR Description

If ImageMagick dependency enabled, this PR adds a new writer to export images to (colored) PNG, JPG or any file format defined in ImageMagick.

Checklist

  • Unit-test of your feature with Catch.
  • Doxygen documentation of the code completed (classes, methods, types, members...)
  • Documentation module page added or updated.
  • New entry in the ChangeLog.md added.
  • No warning raised in Debug cmake mode (otherwise, Travis C.I. will fail).
  • All continuous integration tests pass (Travis & appveyor)

@dcoeurjo
Copy link
Member Author

can someone check this PR ? Travis is having a problem with imagemagick but I'm not able to reproduce the bug..

@dcoeurjo
Copy link
Member Author

help please;)

@JacquesOlivierLachaud
Copy link
Member

The error with imagemagick seems related to a bad config path (some surfing indicates MAGICK_CONFIGURE_PATH but I have not checked it yet). For the internal g++-4.8 compiler error, I don't know. I had also ImageMagick problems on my Mac, I had to reinstall it (another version) and it worked.
I'll have a look.

@kerautret
Copy link
Member

I checked also on my updated Mac and also on older version but also not able to reproduce the bug.
On the Debian server same result. I look it if perhaps by trying other magic version

@kerautret
Copy link
Member

@dcoeurjo I think I got it ;) ! I PR on your branch.
It follows the @JacquesOlivierLachaud mention with an MAGICK_CONFIG_PATH and with an add of the file itself (perhaps we could remove later the fix when the package will be fixed).

kerautret
kerautret requested changes Jan 30, 2018
View reviewed changes
Copy link
Member

@kerautret kerautret left a comment

Choose a reason for hiding this comment

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

Just some small edit can be merged.
I tested in a simple example ImageMagick Reader/Writer with DGtalColor image and all is fine! ;)

src/DGtal/io/writers/MagickWriter.h
Color c = aFunctor( anImage(point) );
Magick::ColorRGB magickc( c.red()/255., c.green()/255., c.blue()/255.) ;
image.pixelColor( point[0] + anImage.domain().lowerBound()[0],
(h-1) - (point[1] + anImage.domain().lowerBound()[1]),
Copy link
Member

Choose a reason for hiding this comment

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

anImage.domain().upperBound()[1]-point[1] ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks , I’ll check

Copy link
Member Author

Choose a reason for hiding this comment

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

that should be the same.

src/DGtal/io/writers/MagickWriter.h Outdated
BOOST_STATIC_ASSERT( (TImage::Domain::dimension == 2) );

/**
* Export an Image with Imagemagick.
Copy link
Member

Choose a reason for hiding this comment

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

ImageMagick

src/DGtal/io/writers/MagickWriter.h Outdated
/**
* Export an Image with Imagemagick.
*
* @param filename name of the output file the suffix is used to select
Copy link
Member

Choose a reason for hiding this comment

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

file the -> file, the ?

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks, fixed

@kerautret
Copy link
Member

Another comment, the integration with the GenericWriter could be nice, I am looking it, let me know if you think that I can PR on your branch or if you prefer another PR.

@dcoeurjo
Copy link
Member Author

Not tested yet but the integration within GenericReader is already there

@dcoeurjo
Copy link
Member Author

Sorry, you meant GenericWriter...

@dcoeurjo @kerautret
Fix @kerautret's comments
e508713 
Co-authored-by: Bertrand Kerautret <bertrand.kerautret@univ-lorraine.fr>
@dcoeurjo
Copy link
Member Author

Comments fixed. I'm merging this PR, thanks.

@dcoeurjo dcoeurjo merged commit a3c6e18 into DGtal-team:master Jan 31, 2018
@kerautret
Copy link
Member

ok fine for me ;)

@dcoeurjo dcoeurjo deleted the MagickWriter branch October 7, 2021 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kerautret kerautret kerautret requested changes

Assignees
No one assigned
Labels
IO-Visualisation ready for review 👍
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@dcoeurjo @JacquesOlivierLachaud @kerautret