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 functions to generate drafts #65

Draft
wants to merge 46 commits into
base: master
Choose a base branch
from

Conversation

GlennWSo
Copy link
Contributor

@GlennWSo GlennWSo commented Jan 19, 2023

relates to: #64

added import helper

  • added draft function for extrusions
  • added draft tests

@GlennWSo
Copy link
Contributor Author

@jimy-byerley this pr is ready for ur input

featrues

draft by groups, for adding draft on extruded(Mesh)
draft by slice, for adding draft on any extrusion
draft by axis, for adding draft any Mesh using Axis as definition of neutral plane
draft extrusion, lazy for creating extrusion and adding a draft.

TODO

  • handle self intersection

testing

u can test on the darft funcs by running:

pytest tests/test_drafts.py

to plot all drafts:

pytest tests/test_drafts.py -v

questions:

draft extrusion, lazy for creating extrusion and adding a draft.

  1. maybe an optimal argument should be added extrusion instead?
    def extrusion(trans, line: Web, alignment:float=0, draft_angle=0) -> Mesh:
  2. should handling self-intersection be a separate pr?

@jimy-byerley
Copy link
Owner

Hello
I have few concerns about the functions here:

The draft is made as a modification of an already generated mesh (the result of an extrusion()) which can lead to different issues:

  • I discourage the creation of an API that creates a mesh, then operate modifications on already present content like in the following example. This is because such API is less convenient to combine (you cannot make a big python expression calling all those functions), plus it often leads to function that ask the user to know about what geometry is already in the mesh

    # weird
    some = generate(...)
    modify_inplace(some)
    return some
    # prefer this
    return modified_clone(generate(...))
    # or this if modified cannot rely on generate, or for performance reasons
    return generate_modified(...)

    In this matter, chamfer() and bevel() are specific cases where I couldn't find a way to do things better than modifying the input mesh and asking the user to provide indices in it .. that's because chamfer() and bevel() really adjusts to their cutted place ... at least these input indices are easy to provide using the groups mechanisms.

  • draft_by_groups() needs the user to know the groups in the mesh (definitely doable, but a bit demanding, the user will have to search the documentation and use Mesh.qualified_groups(), making it less easy to call draft_by_groups())

  • draft_by_groups() needs the input mesh to contain an extrusion of a loop profile, else there won't be a free and a fixed group. This leads to a situation where draft_by_group() represent a very specific case and we prefer generic functions over specific.

  • draft_by_slice() needs a specific order of the points in the buffer of points, which is not good since the way functions generating meshes choose to organize points in their output buffers is an implementation detail. it also needs all the points in the buffer to belong to this mesh (which is not always the case since points buffer are generally shared accross meshes to save memory and copies). One function having a mesh as input should not make assumptions on the points ordering.

  • draft_by_slice() needs the user to provide an index in the buffer of points, this is very demanding since the madcad API tends to make abstraction of the mesh resolution, and we promote to do so in the user's code. using the exact same script but simply changing madcad.settings.primitives['curve_resolution'] can totally change the number of points and thus indices

  • draft_extusion() makes assumption that the points buffer returned by extrusion() is composed of: first half original points, second half moved points. Which in practice is true, but it is an implementation detail of extrusion() and this may change so we shouldn't rely in that fact.

  • draft_by_axis() does not seems to handle the case of curved surfaces, where moving the points to draft can break the geometry

I like however the draft_extrusion() function: easy and simple signature. with no inputs modification. In my opînion, drafts should be like regular madcad generations functions: taking a wire or a surface and returning a mesh.
If I am correct, draft_by_axis() is meant for any mesh, to add drafts where necessary. I like this, but I think this should be made more robust to handle the general case with curved surfaces, weird and tight geometries and so.

I propose the following:

  • make draft_extrusion() implement its own surface generation rather than using extrusion()
  • maybe rename draft_extrusion() to draftside(web/mesh, direction: vec3, angle: float) -> mesh
  • rename draft_by_axis() to drafts(mesh, axis:Axis, angle:float) -> mesh adding draft to a mesh, each place necessary, and handling self-intersection

Sorry for the very long answer. I think your efforts deserved some long explanations of my thinkings

@jimy-byerley
Copy link
Owner

jimy-byerley commented Jan 29, 2023

I forgot a minor point:
the syntax type | type in annotations is not supported before python 3.10, so we should not use it in madcad uintil we stop the support for 3.8 and 3.9 ;)
I usually just put no annotation when multiple types are accepted and just mention it in the docstring

@jimy-byerley jimy-byerley changed the title draft pr add functions to generate drafts Jan 29, 2023
@GlennWSo
Copy link
Contributor Author

I forgot a minor point: the syntax type | type in annotations is not supported before python 3.10, so we should not use it in madcad uintil we stop the support for 3.8 and 3.9 ;) I usually just put no annotation when multiple types are accepted and just mention it in the docstring

oh, i wasn't aware that was new feature of python, ill fix it

@GlennWSo
Copy link
Contributor Author

draft_extusion() makes assumption that the points buffer returned by extrusion() is composed of: first half original points, second half moved points. Which in practice is true, but it is an implementation detail of extrusion() and this may change so we shouldn't rely in that fact.

Yeah I agree for a function to depend on "knowing" the implementation details(detailed dependency) of another this is bad design.
And implementing the extrusion internally for draft_extusion resolves detailed dependency , and presents more optimizations opportunity.

What do think of this alternative solution:
Add draft angle as argument to the existing function: extrusion.
So it new signature becomes:

def extrusion(trans, line: Web, alignment:float=0, draft_angle=0) -> Mesh:

@GlennWSo GlennWSo marked this pull request as draft January 29, 2023 17:33
@GlennWSo
Copy link
Contributor Author

rename draft_by_axis() to drafts(mesh, axis:Axis, angle:float) -> mesh adding draft to a mesh, each place necessary, and handling self-intersection

Ill add some more tests cases of "weird" meshes ;) and take a shoot at "handling self-intersection"

@GlennWSo
Copy link
Contributor Author

GlennWSo commented Feb 8, 2023

TODO

handle dirty/unfinished mesh. Right now, the draft with neutral plane function dont work on meshes that mutated by the show method. I need to investigate why it fails in this case. i think goal is, "draft with a neutral plane" should work even if mesh is non-manifold in various ways

The reason why draft failed in this case was that show([mesh]) mutated the mesh in way that extended mesh with points that where not used by any face. When i first wrote the function i did not consider the case of dirty meshes.

I have now added a test case for the draft function with dirty meshes.

@GlennWSo
Copy link
Contributor Author

GlennWSo commented Feb 8, 2023

the draft function can now deal with dirty meshes :)
I think the pr is ready now

Copy link
Owner

@jimy-byerley jimy-byerley left a comment

Choose a reason for hiding this comment

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

Overall, this PR needs to be polished before to be merged: it needs docs and a bit of module cleanup

return n3 * scale


def edges_with_points(edges, pids, only=True):
Copy link
Owner

Choose a reason for hiding this comment

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

What is the purpose of this function ? it doesn't seems to be used anywhere

@@ -0,0 +1,24 @@
"""
lazy imports for common utilites, globals and containers
Copy link
Owner

Choose a reason for hiding this comment

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

Ideally, the root module madcad already serve that purpose. It exposes most of the end-user functions.
From the user perspective there is not need of an other exposing-only module, except eventually if we add one to expose only types and no functions.
From the developer perspective one more thing is to consider: the module loading order.
At contrary to compiles languages, python is loading modules by executing their code. So imagine the following scenario:

# the user is loading madcad for the first time
import madcad # this will start loading of madcad/__init__.py

# madcad/__init__.py
from .draft import draft   # this actually pause the loading of __init__.py to load draft.py and pick its content

# madcad/draft.py
from madcad import ...   # this finds __init__.py already initialized but partially loaded, so part of its content is not there yet.

For that reason, the madcad submodules should not import from the root module like in that prelude file, but only from other submodules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what i want as developer is be able to import some commonly used "core" stuff to reduce boilerplate code inside test_code and the modules that are not "core". I did not write the prelude.py with the end user in mind

madcad/draft.py Show resolved Hide resolved
tests/test_draft.py Outdated Show resolved Hide resolved
@jimy-byerley
Copy link
Owner

Also for the consistence of the API (and user convenience):

  • draft_edge() should handle the groups from its input Web
  • draft() should return a new Mesh instead of modifying inplace its input. The new mesh can share some of its buffers with the input one, but as most points are modified it makes more sense to clone the input buffer of points and then make the modifications.

@jimy-byerley
Copy link
Owner

I'm trying to figure what exactly does the current implementation of draft() it seems to be dilating the mesh in some way ? 🤔
Given a sphere (which usually in molding only needs draft near its half), it returns the green part
image

@GlennWSo
Copy link
Contributor Author

@jimy-byerley

I'm trying to figure what exactly does the current implementation of draft() it seems to be dilating the mesh in some way ? thinking Given a sphere (which usually in molding only needs draft near its half), it returns the green part image

what behavior do want when applying draft on sphere?

I dont not consider warping effect a bugg, its a feature.
Consider the case: Cube with bevels.

  • The surfaces that have orthogonal face normals to draft direction are moved to the specified draft angle.
  • The cube faces with parallel normals are not affected.
  • The constant radius bevel become variable radius bevels to smoothly bridge the gap.

If you test drafting with neutral plane in some proprietary CAD software like SolidWorks or CATIA on part that have fillets. The fillets become varible radius fillets in a similar way.

@GlennWSo GlennWSo marked this pull request as draft February 14, 2023 12:14
@GlennWSo
Copy link
Contributor Author

GlennWSo commented Feb 14, 2023

As a mechanical engineer i consider it bad practice to add drafts on beveled geometry.
In most cases it makes more sense to add the bevel after the draft. But its nice to if both options are available.

@GlennWSo
Copy link
Contributor Author

i think i understand what behavior u want for the sphere now. Something like a chamfer but in way that caps the draft angles

@GlennWSo
Copy link
Contributor Author

TODO

  • cleanup unused functions
  • remove prelude
  • draft_edge should handle groups/tracks
  • draft() should not mutate input mesh
  • document draft()
  • create draft_split

@GlennWSo
Copy link
Contributor Author

...Given a sphere (which usually in molding only needs draft near its half) ...
ill create seperate functions for this behavior: draft_split
it will behave like a cut and i think it will give u the functionality that your are looking for when casting spheres

@GlennWSo
Copy link
Contributor Author

draft_cuts is perhaps a better name?

@jimy-byerley
Copy link
Owner

My thoughts about drafts on a sphere is something like this:
given a mesh sphere, a neutral direction and a draft angle, the function will expand the surface where the original mesh has a smaller angle to the neutral direction than the given angle alpha. As you said: something that caps the draft angles

draft-simple

There is a self-intersection problem between the expanded flanges, because there is flanges drafting from the bottom and flanges drafting from the top.
In more complex situations, this leads to the following:

draft-complex

The complex cases has to be handled if we are about to add a generic draft(mesh, direction, angle) function in the library, because no function we propose must work only for a subset of its arguments. So this is difficult to acheive but definitely desirable for the library.

@jimy-byerley
Copy link
Owner

draft_cuts is perhaps a better name?

Not sure, as such this function will be a very general function, maybe a general name like draft(mesh, direction, angle) is better 🤔

@GlennWSo
Copy link
Contributor Author

GlennWSo commented Feb 19, 2023

What I was thinking about when I mentioned draft cut was similar to ur drawings but instead of adding material to cap drafts I was thinking remove material.

So add_drafts we could call the behaviour u decribed. cut_drafts would be similar. But it would not be tangent but it can instead persevere the outline at the neutral plane

@GlennWSo
Copy link
Contributor Author

The draft() implementation I wrote behaves like a drafty offset. So maybe we should call it something like that. Perhaps draft_transform

@jimy-byerley
Copy link
Owner

So add_drafts we could call the behaviour u decribed. cut_drafts would be similar. But it would not be tangent but it can instead persevere the outline at the neutral plane

+1 on that

So maybe we should call it something like that. Perhaps draft_transform

perhaps yes

@GlennWSo
Copy link
Contributor Author

An argument for mutation

wanted:

drafts on some sides
partial_draft_top
partial_draft

This is straightforward to do with mutation

cube = cad.brick(width=vec3(1, 2, 1))
draft(cube.group({0, 1}), z_axis, 10, inplace=True)
show([cube])

@GlennWSo
Copy link
Contributor Author

I propose that draft_transform have the fallowing signature:


def draft_transform(mesh: Mesh, axis: Axis, angle: float, inplace=False) -> Mesh:
	"""
	Offsets faces in mesh to set draft angle of orthogonal faces.
	Parallel faces are not effected. The adjustment of intermediate faces are interpolated.

	This funcion perserves topology and suitable for adding drafts on mesh with bevels.
	"""
        ...

inplace toggles mutation

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.

None yet

2 participants