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 gear generator plugin #13

Closed
wants to merge 30 commits into from
Closed

Conversation

Jojain
Copy link
Contributor

@Jojain Jojain commented Mar 27, 2021

Hello,

I have been working on a gear generator plugin which I manage come with a first version.
I invite you to read the readme file to understand a bit more how it works. Nonetheless there is few point to note :
It should close #1

The plugin methods should be automatically linked to cq.Workplane object but for some reason it doesn't work in cq-editor.
In a python interpreter or in jupyter-cadquery it does work so it has something to do with how cq-editor works which I'm not yet really aware of.

There is sets of parameters that lead to wrong geometry. It is especially true for bevel gear which I tried to setup some checks to give the user more info than the BREP_API command not done but there is still some work to do on this. Currently on some sets of parameters cadquery will not render a valid shape but won't raise an error while creating bevel gear. It is due to the teeth narrowing down to 0 width before reaching the requested tooth width.

The rack gear can sometimes not cut the last tooth and the helical rack gear don't cut enough material when the width is too big.

There is more checks and tests that could be done on this plugin but since my time is limited I feel it's better to first share this and to work on improvment later. Since I know some other people wanted to work on gear as well it might aswell find other contributors.

To install this from my branch run the following command :
pip install -e "git+https://github.com/Jojain/cadquery-plugins.git@dev-gear-generator#egg=gear_generator&subdirectory=plugins/gear_generator

@adam-urbanczyk tagging Adam because I don't know if he is managing this repo and he asked me about my bevel gears on the discord

@Jojain Jojain closed this Mar 29, 2021
@Jojain Jojain reopened this Mar 29, 2021
@adam-urbanczyk
Copy link
Member

Great, thanks for sharing this. I'll look into the CQ-editor stuff, but it make take some time. Are you sure that you want register the functions as cq.Workplane methods? I don't think that it is really desired.

@Jojain
Copy link
Contributor Author

Jojain commented Mar 29, 2021

Well, I made them cq.Workplane methods as it was from what I understand what this repo aims to bring to cq.

Now if we are to think about what would be the best way to implement this, I think the best would be to make class of them, eventually to inherit from a Part class (bringing back what have been done in cqparts)

It would be useful for accessing parameters computed inside the function that can be useful for the user (like the gear diameter for example)

Since this repo may holds more part generator plugins like this one, it would maybe be a good idea to discuss the best method to implement it

@jmwright
Copy link
Member

Well, I made them cq.Workplane methods as it was from what I understand what this repo aims to bring to cq.

This repo is fairly new and we're still figuring out best practices. I don't think that monkey-patching cq.Workplane needs to be a requirement, we just want to be intentional about the methods used. As always, we can discuss as a community and adjust as needed.

Now if we are to think about what would be the best way to implement this, I think the best would be to make class of them, eventually to inherit from a Part class (bringing back what have been done in cqparts)

I personally would be fine with this direction. Would the Part class be its own plugin that would then become a dependency of this gear plugin?

@Jojain
Copy link
Contributor Author

Jojain commented Mar 29, 2021

I think so, this would allow every part generator like this plugin to have some common base.
From this we would have to decide what is a part, would it be something quite simple that just holds all the parameters of the part to be easily accessible or maybe something more ?

Since I am quite new to cadquery I haven't used cqparts while it was still alive but I've seen that there is lot of stuff that can be source of inspiration here.

I think the concept of parts objects is something we should have, since it's so common in other cad packages, users would feel more at ease with it i think

@jmwright
Copy link
Member

cqparts tends to have deep abstractions and I don't think that we want to make the Part class overly complex, but just for reference, here's a link to cqparts.Part, which is a subclass of cqparts.Component. https://github.com/jmwright/cqparts/blob/master/src/cqparts/part.py

We could start with the simplest Part class possible. As you've suggested, it could hold generic parameters, and any base functions to make your gear generator work. If we make it a subclass of cq.Assembly, the Part can carry color and location info, and be ready to be used as part of an assembly.

However, I'd like to hear any thoughts from @marcus7070 and @adam-urbanczyk and any additional thoughts on my ideas, @Jojain , before we start implementing things.

@adam-urbanczyk
Copy link
Member

-1 from me on enforcing an OOP design here and using common base class(es). Unless you have a really good reason (e.g. you need to maintain state) I'd propose to just use functions:

make_bevel_gear(...) -> cq.Shape
make_bevel_gear(...) -> cq.Workplane
make_bevel_gear(...) -> cq.Assembly

What you want to return is up to you, but I'd propose to start with cq.Shape as the most universal.

@Jojain
Copy link
Contributor Author

Jojain commented Mar 30, 2021

Note to myself and the eventual reviewers : in the make_gear and make_rack_gear, the param helix_angle isn't actually the helix angle but simply the twist angle, I need to add some math to take this in account properly.

Second point the crown/face gear isn't actually a crown/face gear it's a bevel gear with infinite angle, it looks nice but it's useless, since it don't mesh with anything, I need to do more research to see how it is made.

@Jojain
Copy link
Contributor Author

Jojain commented Apr 7, 2021

I had been thinking about how this need to be implemented and I still think having an OOP implementation is best but I don't think having a more general part class is needed. I'm thinking of just deriving class based on a parent gear class since a lot of parameters are shared among all types of gear, it would clean things up a bit.

Since there is a lot of things I would like to change/improve but that are more time consuming that I expected, do you guys want to merge a version of this plugin with less functionality (I.e maybe just the regular gear) first so there is no rush to add other type of gears or do you prefer to merge it like this even though there is known improvement that are needed ?

@marcus7070 Do you have any thoughts about this ?

@marcus7070
Copy link
Member

If you prefer to write OOP, then a set of gear generator classes would be great. I don't want to enforce a coding style on you in this repo, whatever you want to contribute is fine. Within reason though, please don't inherit from OCP.TopoDS_Shape or something insane like that.

I don't think these need to be Workplane methods. I would be returning a Shape, or possibly an Assembly if you can make a gear train (if it's a gear generator class, then a GearGenerator.build() or similar method would return these types). Even if it's not plugged into the Workplane class, I think gear generators would belong here. Plugins or gear generators are all utility methods that you would use as a step in normal CQ modelling, so I'm fine with them all being put together in here.

I don't think a more general part class is a good idea. I can't imagine it making many tasks easier, and it adds another layer of code that has to be understood by the user.

Since there is a lot of things I would like to change/improve but that are more time consuming that I expected, do you guys want to merge a version of this plugin with less functionality (I.e maybe just the regular gear) first so there is no rush to add other type of gears or do you prefer to merge it like this even though there is known improvement that are needed ?

I would prefer just the regular gear please.

@adam-urbanczyk
Copy link
Member

@Jojain I'm slightly opinionated about overusing OOP. What do you mean by parameter sharing? Is it about similar function signatures or something else? IMO having to call multiple methods (if you have that in mind) is annoying from the user point of view.

@Jojain
Copy link
Contributor Author

Jojain commented Apr 7, 2021

There two reasons why I think OOP is better, the main one :

  • There is a lot of secondary parameters (like gear root, tip and pitch diameter for example) that are computed in the function to create the gear. These parameters might be useful to the user and since it is already computed for the gear generation we might aswell provide them as class attributes to the user rather than having the user recomputing what he needs on his own.

  • The second reason is that it allows creating a base gear class that other gear class could inherits properties. It would be more to organize code and would be transparent to the user. What i have in mind is similar to what i have done in more_selectors plugin.

I don't mind not having the second point but I think the first is important.

@adam-urbanczyk
Copy link
Member

None of the two things require OOP - you can return parameters along your shape and you can structure procedural or functional code to support reuse/composition.

Anyhow, I don't think that we want to enforce particular paradigm as long as things don't get too crazy.

Your comment regarding properties made me think: maybe it would be good to add a metadata field cq.Shape and cq.Workplane.ctx to allow to attach this kind of information to the resulting objects. It already exists for cq.Assembly, for now just to reserve place for future use like defining materials.

@Jojain
Copy link
Contributor Author

Jojain commented Apr 8, 2021

I agree that OOP isn't required to do this. But I think returning a dict of parameters or something similar wouldn't feel really good.

I could even attach theses properties dynamically to the return cq object. But this didn't felt really right to do.

However what you suggested is I think a good idea. It solves the problem of adding another layer on top cq.Shape / cq.Workplane just to link data's to the created shape.

Would this be complicated to implement ? I will look at what is done to the assembly class to get an idea.

@Jojain Jojain mentioned this pull request Apr 22, 2021
@Jojain Jojain closed this Apr 22, 2021
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.

[Plugin Request] Gear Generator
4 participants