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

utils_poly #46

Open
demsham opened this issue Mar 6, 2020 · 6 comments
Open

utils_poly #46

demsham opened this issue Mar 6, 2020 · 6 comments
Assignees
Labels
help wanted Extra attention is needed

Comments

@demsham
Copy link
Collaborator

demsham commented Mar 6, 2020

I was planning to move around some functions, that's why this was not yet renamed.
now there's some confusion as to what goes here and what goes in the utils_vertex.

The utils_vertex currently has the
vertex_ functions (add, subtract etc.)
vertices_list_ functions (input is a v list)
triangle_ functions (input is 3 vertices)

then there's some functions with different inputs and questionable naming like
def vertex_offset_point(v1, v2, v3, offset1, offset2):
def vertex_line_line_intersection(a,b,c,d):

The utils_poly has some functions with vertex input, but a brand new prefix, like normal_
def normal_edge_2d_non_unified(vprev, v):
plus a construct_ function which normally returns meshes except in this case
construct_circle

should we clean this up? ideas?

@demsham
Copy link
Collaborator Author

demsham commented Mar 6, 2020

should the prefix of these functions be consistent with the input..? (or the return?)
should we merge both in one file and be a bit more clear with the prefix?
should we keep two files but move some of the functions? (e.g vertices_list_ functions go in the poly file)

@modellstadt
Copy link
Contributor

If I remember well, poly was supposed to work on 2D polygons.
a: we could skip this.
b: add 2D inside the label?

for example utils_geom2D? or utils_vertex2D?

@modellstadt
Copy link
Contributor

modellstadt commented Mar 6, 2020

regarding confusion in vertex class:
I would perhaps create a new utils_vertex_list or utils_vertices module which could take all utils which work on multiple vertices?
This could also incooperate the utils_poly methods, indicating 2D in the name of the functions

@modellstadt modellstadt added the help wanted Extra attention is needed label Mar 6, 2020
@worbit
Copy link
Collaborator

worbit commented Mar 8, 2020

i was too fast then in just renaming the methods to the new snake_case convention. but anyway, i think so far we did not use any of the methods inside of this module. we can also just hide its import in the init.py file and leave it there until we need it?

@modellstadt
Copy link
Contributor

Shall we close this issue?

@demsham
Copy link
Collaborator Author

demsham commented Mar 16, 2020

Shall we close this issue?

i would keep this a few more days as a reminder. we are still using the same structure in 0.4 but i'm thinking to make it a bit clearer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants