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

Itk image origin fix #1484

Merged
merged 5 commits into from
May 2, 2020
Merged

Itk image origin fix #1484

merged 5 commits into from
May 2, 2020

Conversation

kerautret
Copy link
Member

@kerautret kerautret commented Apr 26, 2020 •
edited
Loading

PR Description

Fix origin image that was not taken into account in class ImageContainerByITKImage.
(all ITK images were always shifted with origin (0,0,0))

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
dcoeurjo requested changes May 1, 2020
View reviewed changes
Copy link
Member

@dcoeurjo dcoeurjo left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Minor comment

ChangeLog.md Outdated Show resolved Hide resolved
@kerautret kerautret merged commit 613333e into DGtal-team:master May 2, 2020
@phcerdan
Copy link
Member

phcerdan commented May 15, 2020 •
edited
Loading

I am getting crashes because of this, and I fear this is partly wrong. GetPixel expects an index, not a physical point (origin).

Before this patch, they were in the Index Space, after this patch, there is a mixture between Index Space and Physical Space.
The ImageRegion iterator iterate over indices, so it doesn't make sense to add origin there.

@kerautret could you provide a minimal example that fails and this patch solves it?

I think this patch should be reverted.

phcerdan added a commit to phcerdan/DGtal that referenced this pull request May 15, 2020
@phcerdan
Revert itk image origin fix
e85586c 
The patch DGtal-team#1484 mixes index space and physical space (with origins) in
ITK.

The iterators work on regions, defined by an `index` and a `size`.
Metadata, like origin, that is defined in the physical space, cannot be
mixed there.

Revert DGtal-team#1484
@phcerdan phcerdan mentioned this pull request May 15, 2020
Merged
5 tasks
phcerdan added a commit to phcerdan/DGtal that referenced this pull request May 15, 2020
@phcerdan
Revert itk image origin fix
60a8fee 
The patch DGtal-team#1484 mixes index space and physical space (with origins) in
ITK.

The iterators work on regions, defined by an `index` and a `size`.
Metadata, like origin, that is defined in the physical space, cannot be
mixed there.

Revert DGtal-team#1484
phcerdan added a commit to phcerdan/DGtal that referenced this pull request May 18, 2020
@phcerdan
Add a domainShift in ImageContainerByITKImage
b406315 
Allowing to shift the domain, without affecting the indexing
of the underlying ITK image.

Also add basic conversions from index points to physical points.

The `domainShift` parameter can only be set with the existing
function`updateDomain`. It defaults to `Zero`.

The implementation provided in DGtal-team#1484 was untested,
and I would say it goes out of the buffer of the
ITK region when using `operator()` or `setValue`.

This patch introduces a conversion function between what I call a
`domainPoint` (a point in the DGtal domain defined by `lowerBound` and `upperBound`),
and an `indexPoint` which is a valid point in the ITK region.

These two are equivalent when `domainShift` is Zero (the default).
phcerdan added a commit to phcerdan/DGtal that referenced this pull request May 18, 2020
@phcerdan
Add a domainShift in ImageContainerByITKImage
bfbc457 
Allowing to shift the domain, without affecting the indexing
of the underlying ITK image.

Also add basic conversions from index points to physical points.

The `domainShift` parameter can only be set with the existing
function`updateDomain`. It defaults to `Zero`.

The implementation provided in DGtal-team#1484 was untested,
and I would say it goes out of the buffer of the
ITK region when using `operator()` or `setValue`.

This patch introduces a conversion function between what I call a
`domainPoint` (a point in the DGtal domain defined by `lowerBound` and `upperBound`),
and an `indexPoint` which is a valid point in the ITK region.

These two are equivalent when `domainShift` is Zero (the default).
phcerdan added a commit to phcerdan/DGtal that referenced this pull request May 18, 2020
@phcerdan
Add a domainShift in ImageContainerByITKImage
0184b0e 
Allowing to shift the domain, without affecting the indexing
of the underlying ITK image.

Also add basic conversions from index points to physical points.

The `domainShift` parameter can only be set with the existing
function`updateDomain`. It defaults to `Zero`.

The implementation provided in DGtal-team#1484 was untested,
and I would say it goes out of the buffer of the
ITK region when using `operator()` or `setValue`.

This patch introduces a conversion function between what I call a
`domainPoint` (a point in the DGtal domain defined by `lowerBound` and `upperBound`),
and an `indexPoint` which is a valid point in the ITK region.

These two are equivalent when `domainShift` is Zero (the default).
phcerdan added a commit to phcerdan/DGtal that referenced this pull request May 18, 2020
@phcerdan
Add a domainShift in ImageContainerByITKImage
25f3431 
Allowing to shift the domain, without affecting the indexing
of the underlying ITK image.

Also add basic conversions from index points to physical points.

The `domainShift` parameter can only be set with the existing
function`updateDomain`. It defaults to `Zero`.

The implementation provided in DGtal-team#1484 was untested,
and I would say it goes out of the buffer of the
ITK region when using `operator()` or `setValue`.

This patch introduces a conversion function between what I call a
`domainPoint` (a point in the DGtal domain defined by `lowerBound` and `upperBound`),
and an `indexPoint` which is a valid point in the ITK region.

These two are equivalent when `domainShift` is Zero (the default).
phcerdan added a commit to phcerdan/DGtal that referenced this pull request May 18, 2020
@phcerdan
Add a domainShift in ImageContainerByITKImage
6e63d8b 
Allowing to shift the domain, without affecting the indexing
of the underlying ITK image.

Also add basic conversions from index points to physical points.

The `domainShift` parameter can only be set with the existing
function`updateDomain`. It defaults to `Zero`.

The implementation provided in DGtal-team#1484 was untested,
and I would say it goes out of the buffer of the
ITK region when using `operator()` or `setValue`.

This patch introduces a conversion function between what I call a
`domainPoint` (a point in the DGtal domain defined by `lowerBound` and `upperBound`),
and an `indexPoint` which is a valid point in the ITK region.

These two are equivalent when `domainShift` is Zero (the default).

It also effectively reverts DGtal-team#1484 that mixes index space and physical
space (with origins) in ITK.
phcerdan added a commit to phcerdan/DGtal that referenced this pull request May 18, 2020
@phcerdan
Add a domainShift in ImageContainerByITKImage
fd8e3d1 
Allowing to shift the domain, without affecting the indexing
of the underlying ITK image.

Also add basic conversions from index points to physical points.

The `domainShift` parameter can only be set with the existing
function`updateDomain`. It defaults to `Zero`.

The implementation provided in DGtal-team#1484 was untested,
and I would say it goes out of the buffer of the
ITK region when using `operator()` or `setValue`.

This patch introduces a conversion function between what I call a
`domainPoint` (a point in the DGtal domain defined by `lowerBound` and `upperBound`),
and an `indexPoint` which is a valid point in the ITK region.

These two are equivalent when `domainShift` is Zero (the default).

It also effectively reverts DGtal-team#1484 that mixes index space and physical
space (with origins) in ITK.
phcerdan added a commit to phcerdan/DGtal that referenced this pull request May 18, 2020
@phcerdan
Add a domainShift in ImageContainerByITKImage
f4f4ca4 
Allowing to shift the domain, without affecting the indexing
of the underlying ITK image.

Also add basic conversions from index points to physical points.

The `domainShift` parameter can only be set with the existing
function`updateDomain`. It defaults to `Zero`.

The implementation provided in DGtal-team#1484 was untested,
and I would say it goes out of the buffer of the
ITK region when using `operator()` or `setValue`.

This patch introduces a conversion function between what I call a
`domainPoint` (a point in the DGtal domain defined by `lowerBound` and `upperBound`),
and an `indexPoint` which is a valid point in the ITK region.

These two are equivalent when `domainShift` is Zero (the default).

It also effectively reverts DGtal-team#1484 that mixes index space and physical
space (with origins) in ITK.
phcerdan added a commit to phcerdan/DGtal that referenced this pull request May 18, 2020
@phcerdan
Add a domainShift in ImageContainerByITKImage
0714c18 
Allowing to shift the domain, without affecting the indexing
of the underlying ITK image.

Also add basic conversions from index points to physical points.

The `domainShift` parameter can only be set with the existing
function`updateDomain`. It defaults to `Zero`.

The implementation provided in DGtal-team#1484 was untested,
and I would say it goes out of the buffer of the
ITK region when using `operator()` or `setValue`.

This patch introduces a conversion function between what I call a
`domainPoint` (a point in the DGtal domain defined by `lowerBound` and `upperBound`),
and an `indexPoint` which is a valid point in the ITK region.

These two are equivalent when `domainShift` is Zero (the default).

It also effectively reverts DGtal-team#1484 that mixes index space and physical
space (with origins) in ITK.
phcerdan added a commit to phcerdan/DGtal that referenced this pull request May 19, 2020
@phcerdan
Add a domainShift in ImageContainerByITKImage
5ba063f 
Allowing to shift the domain, without affecting the indexing
of the underlying ITK image.

Also add basic conversions from index points to physical points.

The `domainShift` parameter can only be set with the existing
function`updateDomain`. It defaults to `Zero`.

The implementation provided in DGtal-team#1484 was untested,
and I would say it goes out of the buffer of the
ITK region when using `operator()` or `setValue`.

This patch introduces a conversion function between what I call a
`domainPoint` (a point in the DGtal domain defined by `lowerBound` and `upperBound`),
and an `indexPoint` which is a valid point in the ITK region.

These two are equivalent when `domainShift` is Zero (the default).

It also effectively reverts DGtal-team#1484 that mixes index space and physical
space (with origins) in ITK.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dcoeurjo dcoeurjo dcoeurjo approved these changes

Assignees
No one assigned
Labels
BugFix waiting author feedbacks
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kerautret @phcerdan @dcoeurjo