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

Raster to Point Cloud Filters #2121

Merged
merged 5 commits into from May 15, 2018

Conversation

Projects
None yet
2 participants
@rinkk
Copy link
Member

rinkk commented May 11, 2018

Two VTK filters to represent raster data as point clouds + integration into Data Explorer

  1. Varying desity based on pixel value
  2. Representing pixel values as surface
init();
}

VtkCompositeImageToPointCloudFilter::~VtkCompositeImageToPointCloudFilter() = default;

This comment has been minimized.

@endJunction

endJunction May 12, 2018

Member

This can be moved into the header, so the reader immediately sees, that the implementation is the default one...

This comment has been minimized.

@rinkk

rinkk May 14, 2018

Author Member

How do it match this with ~VtkImageDataToPointCloudFilter() override; specified in the header file?

✔️ (removed override)

/// Creates the point objects based on the parameters set by the user
double VtkImageDataToPointCloudFilter::getRandomNumber(double const& min, double const& max) const
{
return ((double)rand() / RAND_MAX) * (max - min) + min;

This comment has been minimized.

@endJunction

endJunction May 12, 2018

Member

Better to use std::uniform_real_distribution. See example on that page for details.

This comment has been minimized.

@rinkk

rinkk May 14, 2018

Author Member

✔️ added std:: as discussed but keeping rand

assert(p >= low && p <= high);
double const r = (p - low) / (high - low);
return static_cast<std::size_t>(
(MaxNumberOfPointsPerCell - MinNumberOfPointsPerCell) * pow(r, gamma) +

This comment has been minimized.

@endJunction

endJunction May 12, 2018

Member

std::pow

This comment has been minimized.

@rinkk

rinkk May 14, 2018

Author Member

✔️

/// Returns a random number in [min, max]
double getRandomNumber(double const& min, double const& max) const;

/// Interpolates the required number of points

This comment has been minimized.

@endJunction

endJunction May 12, 2018

Member

Mentioning the type of interpolation would be nice.

This comment has been minimized.

@rinkk

rinkk May 14, 2018

Author Member

✔️ Added comment.


private:
VtkImageDataToPointCloudFilter(const VtkImageDataToPointCloudFilter&) = delete;
void operator=(const VtkImageDataToPointCloudFilter&) = delete;

This comment has been minimized.

@endJunction

endJunction May 12, 2018

Member

I'm not sure if you have to "delete" the move ctor and assignments too.

This comment has been minimized.

@rinkk

rinkk May 14, 2018

Author Member

I don't know either. This is what we did with all other filters we implemented ourselves. However, I don't remember the exact reasons. Anyway, the way it is now is consistent with the rest of VtkVis.

(That being said, if you'd rather have the lines removed, I can do that, too.)

Removed (as discussed) ✔️

return 1;
}

int VtkImageDataToSurfacePointsFilter::RequestData(

This comment has been minimized.

@endJunction

endJunction May 12, 2018

Member

Parts of the code look very similar to the implementation of int VtkImageDataToPointCloudFilter::RequestData; would it be possible to reduce the code duplication?

This comment has been minimized.

@rinkk

rinkk May 14, 2018

Author Member

Originally I wanted to put both algorithms into the same filter. But then I noticed that the actual algorithms are fairly different. The set of parameters is completely different. One filter needs density information, the other doesn't. One interpolates point positions using a function, the other using a raster. I know that it looks similar but it isn't really.

@rinkk rinkk force-pushed the rinkk:Raster2PointCloudFilter branch from b69842b to ab9d448 May 14, 2018

vtkInformationVector* outputVector) override;

private:
/// Returns a random number in [min, max]

This comment has been minimized.

@endJunction

endJunction May 14, 2018

Member

Misplaced comment

}
}

/// Creates the point objects based on the parameters set by the user

This comment has been minimized.

@endJunction

endJunction May 14, 2018

Member

Misplaced comment.

This comment has been minimized.

@rinkk

rinkk May 15, 2018

Author Member

✔️ (no idea what happened here)

double MaxHeight;
vtkIdType MinNumberOfPointsPerCell;
vtkIdType MaxNumberOfPointsPerCell;
bool IsLinear;

This comment has been minimized.

@endJunction

endJunction May 14, 2018

Member

Overlooked it last time, the variable names usually starts with lower-case letters, capital case is would be for types. I see, that the vtk has a different conventions. Choose, what you think would fit both worlds best.

This comment has been minimized.

@rinkk

rinkk May 15, 2018

Author Member

It needs to start with a capital letter. Otherwise the automatically generated get- and set-functions won't work. It's similar to Qt were class-variables starting with an underscore mess with automatically generated methods.

@endJunction
Copy link
Member

endJunction left a comment

After fixing the two misplaced comments

@endJunction endJunction merged commit d962af6 into ufz:master May 15, 2018

1 of 2 checks passed

continuous-integration/jenkins/pr-merge This commit cannot be built
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@rinkk rinkk deleted the rinkk:Raster2PointCloudFilter branch Oct 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.