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

Spatial_Engine: Add query dominant vector #2539

Merged
merged 10 commits into from
Jul 19, 2021

Conversation

tiagogrossi
Copy link
Contributor

Issues addressed by this PR

Closes #2537

This new method queries IElement1D and IElement2D for their dominant vector (orientation). The logic is very simple, it groups all similar vectors and sort them by summed lengths. The longest length is the dominant vector and hence it's selected. The method also allows for orthogonal vector priority, where if true it tries to return orthogonal vectors, and a angle discrepancy tolerance when grouping vectors.

Test files

Test file is here. I have not included a GH proof because quite honestly I don't even know how to do it with standard nodes.

Changelog

Adds BH.Engine.Spatial.Query.DominantVector

@tiagogrossi tiagogrossi added the type:feature New capability or enhancement label Jul 12, 2021
@tiagogrossi tiagogrossi requested a review from pawelbaran July 12, 2021 16:28
@tiagogrossi tiagogrossi self-assigned this Jul 12, 2021
Copy link
Member

@pawelbaran pawelbaran left a comment

Choose a reason for hiding this comment

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

As a first iteration the code looks good, although I have a few objections as well some questions and general remarks - please see the code comments for details.

Spatial_Engine/Query/DominantVector.cs Outdated Show resolved Hide resolved
Spatial_Engine/Query/DominantVector.cs Outdated Show resolved Hide resolved
Spatial_Engine/Query/DominantVector.cs Outdated Show resolved Hide resolved
Spatial_Engine/Query/DominantVector.cs Outdated Show resolved Hide resolved
Spatial_Engine/Query/DominantVector.cs Outdated Show resolved Hide resolved
Spatial_Engine/Query/DominantVector.cs Outdated Show resolved Hide resolved
return dominantVector;

//filter grouped vectors to find only curves that are X = 0 or 1 (horizontal or vertical lines)
var orthogonalVectors = groupByNormal.Where(x => x.First().Normalise().X == 0 || x.First().Normalise().X == 1).ToList();
Copy link
Member

Choose a reason for hiding this comment

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

You should use some tolerance here, otherwise you can end up with 1 != 1 due to numerical noise.

Spatial_Engine/Query/DominantVector.cs Outdated Show resolved Hide resolved
Spatial_Engine/Query/DominantVector.cs Outdated Show resolved Hide resolved
Spatial_Engine/Query/DominantVector.cs Outdated Show resolved Hide resolved
@tiagogrossi tiagogrossi requested a review from epignatelli as a code owner July 15, 2021 16:05
Copy link
Member

@pawelbaran pawelbaran left a comment

Choose a reason for hiding this comment

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

I found one naming issue which I believe would be good to fix, plus I gave I tried to refactor the code for sport - as mentioned in the comments, please feel free to ignore, I did it more for learning/knowledge sharing than anything else.

Spatial_Engine/Query/DominantVector.cs Outdated Show resolved Hide resolved
Spatial_Engine/Query/DominantVector.cs Outdated Show resolved Hide resolved
Spatial_Engine/Query/DominantVector.cs Outdated Show resolved Hide resolved
@tiagogrossi
Copy link
Contributor Author

@BHoMBot check required

@bhombot-ci
Copy link

bhombot-ci bot commented Jul 16, 2021

@tiagogrossi to confirm, the following checks are now queued:

  • code-compliance
  • documentation-compliance
  • project-compliance
  • core
  • null-handling
  • serialisation
  • installer
  • versioning

There are 51 requests in the queue ahead of you.

pawelbaran
pawelbaran previously approved these changes Jul 16, 2021
Copy link
Member

@pawelbaran pawelbaran left a comment

Choose a reason for hiding this comment

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

LGTM now 👍

@pawelbaran
Copy link
Member

@BHoMBot check required

@bhombot-ci
Copy link

bhombot-ci bot commented Jul 16, 2021

@pawelbaran to confirm, the following checks are now queued:

  • code-compliance
  • documentation-compliance
  • project-compliance
  • core
  • null-handling
  • serialisation
  • installer
  • versioning

There are 112 requests in the queue ahead of you.

@pawelbaran
Copy link
Member

@BHoMBot check required

@bhombot-ci
Copy link

bhombot-ci bot commented Jul 19, 2021

@pawelbaran to confirm, the following checks are now queued:

  • code-compliance
  • documentation-compliance
  • project-compliance
  • core
  • null-handling
  • serialisation
  • installer
  • versioning

@pawelbaran
Copy link
Member

Looks like there is a few compliance issues to resolve @tiagogrossi

@tiagogrossi
Copy link
Contributor Author

@BHoMBot check required

@bhombot-ci
Copy link

bhombot-ci bot commented Jul 19, 2021

@tiagogrossi to confirm, the following checks are now queued:

  • code-compliance
  • documentation-compliance
  • project-compliance
  • core
  • null-handling
  • serialisation
  • installer
  • versioning

There are 71 requests in the queue ahead of you.

pawelbaran
pawelbaran previously approved these changes Jul 19, 2021
Copy link
Member

@pawelbaran pawelbaran left a comment

Choose a reason for hiding this comment

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

Still looks good 👍

@tiagogrossi
Copy link
Contributor Author

@BHoMBot check null-handling

@bhombot-ci
Copy link

bhombot-ci bot commented Jul 19, 2021

@tiagogrossi to confirm, the following checks are now queued:

  • null-handling

@tiagogrossi
Copy link
Contributor Author

@BHoMBot check required

@bhombot-ci
Copy link

bhombot-ci bot commented Jul 19, 2021

@tiagogrossi to confirm, the following checks are now queued:

  • code-compliance
  • documentation-compliance
  • project-compliance
  • core
  • null-handling
  • serialisation
  • installer
  • versioning

@tiagogrossi
Copy link
Contributor Author

@BHoMBot check copyright-compliance

@bhombot-ci
Copy link

bhombot-ci bot commented Jul 19, 2021

@tiagogrossi to confirm, the following checks are now queued:

  • copyright-compliance

There are 11 requests in the queue ahead of you.

@tiagogrossi
Copy link
Contributor Author

@BHoMBot check dataset-compliance

@bhombot-ci
Copy link

bhombot-ci bot commented Jul 19, 2021

@tiagogrossi to confirm, the following checks are now queued:

  • dataset-compliance

There are 13 requests in the queue ahead of you.

@tiagogrossi
Copy link
Contributor Author

@BHoMBot check ready-to-merge

@bhombot-ci
Copy link

bhombot-ci bot commented Jul 19, 2021

@tiagogrossi to confirm, the following checks are now queued:

  • ready-to-merge

There are 15 requests in the queue ahead of you.

Copy link
Member

@pawelbaran pawelbaran left a comment

Choose a reason for hiding this comment

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

...and still looks good!

@pawelbaran
Copy link
Member

@BHoMBot check ready-to-merge

@bhombot-ci
Copy link

bhombot-ci bot commented Jul 19, 2021

@pawelbaran to confirm, the following checks are now queued:

  • ready-to-merge

@al-fisher al-fisher merged commit 07cee29 into master Jul 19, 2021
@al-fisher al-fisher deleted the Spatial_Engine-#2537-AddQueryDominantVector branch July 19, 2021 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature New capability or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Spatial_Engine: Add method to query an element's dominant orientation
3 participants