Skip to content

Commit

Permalink
Address comments from PR #3046
Browse files Browse the repository at this point in the history
* Removed single line `if` statements
* Simplified clamping `position` to use `std::min`
  • Loading branch information
MattCsencsits committed Apr 27, 2020
1 parent 91206b8 commit 7bde55a
Showing 1 changed file with 21 additions and 2 deletions.
23 changes: 21 additions & 2 deletions filters/ColorinterpFilter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -138,10 +138,14 @@ void ColorinterpFilter::prepared(PointTableRef table)
PointLayoutPtr layout(table.layout());
m_interpDim = layout->findDim(m_interpDimString);
if (m_interpDim == Dimension::Id::Unknown)
{
throwError("Dimension '" + m_interpDimString + "' does not exist.");
}
if (!std::isnan(m_min) && !std::isnan(m_max) && m_max <= m_min)
{
throwError("Specified 'minimum' value must be less than "
"'maximum' value.");
}
}


Expand All @@ -157,7 +161,9 @@ void ColorinterpFilter::ready(PointTableRef table)
m_raster = openRamp(m_colorramp);
gdal::GDALError err = m_raster->open();
if (err != gdal::GDALError::None && err != gdal::GDALError::NoTransform)
{
throwError(m_raster->errorMsg());
}

log()->get(LogLevel::Debug) << getName() << " raster connection: " <<
m_raster->filename() << std::endl;
Expand Down Expand Up @@ -247,9 +253,13 @@ void ColorinterpFilter::filter(PointView& view)
}

if (std::isnan(m_min))
{
m_min = summary.minimum();
}
if (std::isnan(m_max))
{
m_max = summary.maximum();
}
}

PointRef point(view, 0);
Expand All @@ -264,7 +274,9 @@ void ColorinterpFilter::filter(PointView& view)
bool ColorinterpFilter::pipelineStreamable() const
{
if (std::isnan(m_min) || std::isnan(m_max))
{
return false;
}
return Streamable::pipelineStreamable();
}

Expand All @@ -273,26 +285,33 @@ bool ColorinterpFilter::processOne(PointRef& point)
{
double v = point.getFieldAs<double>(m_interpDim);

if (m_clamp) v = pdal::Utils::clamp(v, m_min, m_max);
if (m_clamp)
{
v = Utils::clamp(v, m_min, m_max);
}

// Don't color points that aren't in the min/max range
// unless they've been clamped. Allow v == m_max so that
// if the user wants to clamp all values outside m_min
// and m_max the values greater than m_max are colored
// as expected.
if (v < m_min || v > m_max)
{
return true;
}

double factor = (v - m_min) / (m_max - m_min);
size_t img_width = m_redBand.size();
size_t position = size_t(std::floor(factor * img_width));

// Handle the case that v == m_max (position == img_width) by clamping
// position to img_width - 1
position = pdal::Utils::clamp(position, 0UL, img_width - 1);
position = std::min(position, img_width - 1);

if (m_invertRamp)
{
position = (img_width - 1) - position;
}

point.setField(Dimension::Id::Red, m_redBand[position]);
point.setField(Dimension::Id::Blue, m_blueBand[position]);
Expand Down

3 comments on commit 7bde55a

@abellgithub
Copy link
Contributor

Choose a reason for hiding this comment

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

You misunderstood. The only thing I was trying to prevent was the code on line 276 where the predicate and action were on the same line. You didn't need to put parens around everything. Leave it as is or change. Up to you.

@MattCsencsits
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. I interpreted your message as wanting to avoid the future bug when someone adds more lines expecting them to be in the if-clause but they are not.
This aspect of if-clauses doesn't seem to be called out explicitly by the coding style. I'm inclined to leave it as-is if that is alright. No big deal to revert and re-do as intended if not.

@abellgithub
Copy link
Contributor

Choose a reason for hiding this comment

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

There are a few files that use the format:

if () do_something();

but not many.

We don't have coding rules, as such, but we ask people to try to follow the style of what's existing.

Please sign in to comment.