dcowden / cadquery

CadQuery-- a parametric cad script framework
Other
432 stars 56 forks source link

Stack modified retroactively #278

Closed adam-urbanczyk closed 6 years ago

adam-urbanczyk commented 6 years ago

As you know I am working on a PyQT based GUI. There is some progress as you can see below:

obraz

I implemented a rudimentary CQ stack inspector, and to my surprise all the solid objects are modified retroactively (see below). This causes the stack inspector to always show the same (i.e. final object) as the parent object is always overwritten by the newest.

@dcowden @jmwright Is this a bug or a feature? I cannot justify to myself why would one want to have such a behavior. Removing those lines gives me some NOKs FAILED (failures=2, errors=3) but I think the tests are easily fixable.

For reference, here are the lines in question:

https://github.com/dcowden/cadquery/blob/0e1002e8db3cd7b7dbbb08415acba9eb0ed268e9/cadquery/cq.py#L194 https://github.com/dcowden/cadquery/blob/0e1002e8db3cd7b7dbbb08415acba9eb0ed268e9/cadquery/cq.py#L821 https://github.com/dcowden/cadquery/blob/0e1002e8db3cd7b7dbbb08415acba9eb0ed268e9/cadquery/cq.py#L852 https://github.com/dcowden/cadquery/blob/0e1002e8db3cd7b7dbbb08415acba9eb0ed268e9/cadquery/cq.py#L891 https://github.com/dcowden/cadquery/blob/0e1002e8db3cd7b7dbbb08415acba9eb0ed268e9/cadquery/cq.py#L1852 https://github.com/dcowden/cadquery/blob/0e1002e8db3cd7b7dbbb08415acba9eb0ed268e9/cadquery/cq.py#L2341

dcowden commented 6 years ago

Hi, this is a feature. In the current cq implementation, it was intentional that references to old objects are updated as modifications occur. It was done that way to mimic as closely as possible a cad GUI, in which there is only one version of a given solid object in view.

I'm open to changing it, because nearly all people who have enountered it are confused. It seemed like a good idea to the time ;)

dcowden commented 6 years ago

By the way, in another note that is completely awesome looking

adam-urbanczyk commented 6 years ago

@dcowden Thanks. I am for modifying then. It will make the "object inspection" more straightforward

Still I do not get it to be honest. I assume that the current view/state is in .objects and the previous state would be .parent.objects and so on. What does modifying the parent give actually?

dcowden commented 6 years ago

There we two benefits imagined:

(1) even old reveferences point to the latest object, so you don't have to worry about which refs you keep up with,and

(2) if you have a long script, you don't have to worry about lots and lots of copies of objects to keep consuming memory.

Like I said, I was convince more back then it was a good idea. It was like 5 years ago now so some of my thinking has been lost as well. It seems like there were some stronger arguments, but I can't remember them

adam-urbanczyk commented 6 years ago

Thanks, clear!

dcowden commented 6 years ago

Ok, so are you just going to reverse those on your branch?

Also, how do you want to proceed with incorporating your version into main? Do you want me to create a branch , which you would merge I to or?

adam-urbanczyk commented 6 years ago

Yeah I am going to remove those lines and fix the test cases on my branch.

Not sure on the second topic

WizardUli commented 6 years ago

@adam-urbanczyk I too was baffled by this behavior when I first encountered it. I even asked about it on CQ google groups forums https://groups.google.com/forum/#!topic/cadquery/-zrySyqkCTI

After some deliberation I decided that creating a fork without those lines and maintaining it by occasionally re-basing it on top of CQ's master would be much less work than thinking and working around it. https://github.com/Grawp/cadquery/tree/combine_semantic_change

So if you're going to remove it on your OCC branch, it's good news to me, because I've been slowly starting to experiment with the branch recently and I've been planning to fork it soon just for the cause of removing those lines. Now I don't have to do that so it seems :)

adam-urbanczyk commented 6 years ago

@Grawp I already pushed those changes to my cq1_pythonocc branch. Let me know what you think.