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

Add polygon functions #2547

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

joannacirillo
Copy link
Contributor

@joannacirillo joannacirillo commented Aug 10, 2022

Context

Translation of C++ functions of vtkPolygon.
Note: this PR is to be merged after PR #2532 as it uses functions implemented in the latter.

Results

Implementation of

  • computeArea
  • computeCentroid
  • isConvex
  • distanceToPolygon
  • interpolateFunctions
  • intersectWithLine
  • intersectPolygonWithPolygon
  • intersectConvex2DCell

Each of them are also exported as static functions.

Changes

The computeNormal function has been replaced by the static function getNormal exported in the publicAPI as computeNormal.

  • Documentation and TypeScript definitions were updated to match those changes

PR and Code Checklist

  • semantic-release commit messages
  • Run npm run reformat to have correctly formatted code

Testing

  • This change adds or fixes unit tests
  • Tested environment:
    • vtk.js: 25.7.0
    • OS: Windows 10
    • Browser: Firefox 103.0.1

@joannacirillo joannacirillo changed the title Centroid computation Add polygon functions Aug 10, 2022
@joannacirillo
Copy link
Contributor Author

joannacirillo commented Aug 10, 2022

@luciemac @finetjul @DavidBerger98 The pointInPolygon function implemented in #2532 accepts vtkPoints along with an id list as parameter, I was wondering whether it should also be the case for the newly implemented static functions. Maybye those ids could be passed as optional parameters too.
Besides, the setPoints might be removed and remplaced by the initialize function cell that however needs a point ids list.

@joannacirillo joannacirillo force-pushed the centroid-computation branch 3 times, most recently from d1bff5e to a436863 Compare August 12, 2022 14:06
Sources/Common/DataModel/Polygon/index.js Outdated Show resolved Hide resolved
Sources/Common/DataModel/Polygon/index.js Outdated Show resolved Hide resolved
Sources/Common/DataModel/Polygon/index.js Outdated Show resolved Hide resolved
Sources/Common/DataModel/Polygon/index.js Outdated Show resolved Hide resolved
Sources/Common/DataModel/Polygon/index.js Outdated Show resolved Hide resolved
Sources/Common/DataModel/Polygon/index.js Outdated Show resolved Hide resolved
Sources/Common/DataModel/Polygon/index.js Outdated Show resolved Hide resolved
Sources/Common/DataModel/Polygon/index.js Outdated Show resolved Hide resolved
Sources/Common/DataModel/Polygon/Constants.js Show resolved Hide resolved
Sources/Common/DataModel/Polygon/Constants.js Show resolved Hide resolved
@joannacirillo joannacirillo force-pushed the centroid-computation branch 5 times, most recently from 28c5dea to 7b85be5 Compare August 23, 2022 15:50
@joannacirillo joannacirillo marked this pull request as ready for review August 23, 2022 15:51
@joannacirillo joannacirillo force-pushed the centroid-computation branch 7 times, most recently from e4ca963 to 578e9d4 Compare August 25, 2022 07:03
Translation from cpp implementation and add test to those functions
Create functions as static functions and in publicAPI
Fix pointInPolygon and triangulate to return the triangles
Updated ts definition
Sources/Common/Core/PriorityQueue/index.js Show resolved Hide resolved
Sources/Common/DataModel/Polygon/Constants.js Show resolved Hide resolved
Sources/Common/DataModel/Polygon/index.d.ts Show resolved Hide resolved
Sources/Common/DataModel/Polygon/index.d.ts Show resolved Hide resolved
Sources/Common/DataModel/Polygon/index.d.ts Show resolved Hide resolved
model.points.getPoint(idsToHandle[i], current);
const next = [0, 0, 0];
model.points.getPoint(
toCorrectId(idsToHandle[toCorrectId(i + 1, idsToHandle.length)], numPts),
Copy link
Member

Choose a reason for hiding this comment

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

getPoint(
  nextPoint(i, idsToHandle, numPts),
  next
);
...
getPoint(
  previousPoint(i, idsToHandle, numPts),
  previous
);

firstPoint: null,
pointCount: 0,
tris: [],
points: [],
Copy link
Member

Choose a reason for hiding this comment

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

model.points is actually defined in vtkCell as vtkPoints. I doubt it should be [] here

publicAPI.setPoints = (points, pointsIds) => {
let pointsData = points;
if (Array.isArray(pointsData[0])) {
pointsData = points.flat(2);
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't it be points.flat() instead?

} else {
model.pointsIds = pointsIds;
}
publicAPI.computeNormal();
Copy link
Member

Choose a reason for hiding this comment

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

should probably call modified()

}

publicAPI.computeNormal = () => getNormal(model.points, model.normal);
Copy link
Member

Choose a reason for hiding this comment

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

here you ignore model.pointIds, are you sure it is intended ?

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

Successfully merging this pull request may close these issues.

2 participants