Code review of merged Link3 branch

Here's the place for discussion related to coding in FreeCAD, C++ or Python. Design, interfaces and structures.
triplus
Posts: 9278
Joined: Mon Dec 12, 2011 4:45 pm

Re: Code review of merged Link3 branch

Postby triplus » Tue Nov 19, 2019 12:03 am

As @realthunder said the ability of overriding all properties would likely be too complex to achieve, i guess "freeze property" could help with that. That is you would first create a link feature and change its "freeze property" to True. After you could change the linked feature parameters and create a new link. You now have two link features, each with different geometry.

The question i guess is, on how much sense and on how hard would it be, to port the "freeze property" from Assembly 3, and to make it a "core link" functionality.
toralf
Posts: 44
Joined: Fri May 03, 2019 3:54 am

Re: Code review of merged Link3 branch

Postby toralf » Tue Nov 19, 2019 1:04 am

I believe that overriding all properties is not what is requested. Just some parameters of the model. IMHO it would be even acceptable if it is necessary to specify these parameters similar to the OPP-like topo naming approach that realthunder introduced. Meaning the author of the original model specifies which parameters are public (could be changed via link). Then the author could ensure that the model works with a certain range of these parameters. And if with a max/min limit for these public parameters the abuse of the model would be very rare.
realthunder
Posts: 1408
Joined: Tue Jan 03, 2017 10:55 am

Re: Code review of merged Link3 branch

Postby realthunder » Thu Nov 21, 2019 10:51 pm

Wow, I am certainly lagging behind with all this interesting discussion. I am working on another big PR now, so sorry for the MIA. I agree the usefulness of a general way of parametric configuration. The question is how to present that capability to user/developer. I'll get back to it once I sort out the PR at hand.
Try Assembly3 (latest version 0.10.2) along with my custom build of FreeCAD at here.
And if you'd like to show your support, you can donate through patreon, liberapay, or paypal
wmayer
Site Admin
Posts: 15501
Joined: Thu Feb 19, 2009 10:32 am

Re: Code review of merged Link3 branch

Postby wmayer » Thu Nov 21, 2019 11:18 pm

I am working on another big PR now
Why this again? Can't this be divided into smaller parts? By having a quick look just at the commit messages I have the impression that it's a potpourri of changes here and changes there that are not related at all.
IMO, that's not the way PRs should be used. Instead one PR should contain only the changes for one improvement or new feature but it should not try to handle 100 different issues at the same time.
Last edited by wmayer on Mon Nov 25, 2019 2:09 pm, edited 1 time in total.
Reason: Add missing "not"
realthunder
Posts: 1408
Joined: Tue Jan 03, 2017 10:55 am

Re: Code review of merged Link3 branch

Postby realthunder » Thu Nov 21, 2019 11:40 pm

wmayer wrote:
Thu Nov 21, 2019 11:18 pm
I am working on another big PR now
Why this again? Can't this be divided into smaller parts? By having a quick look just at the commit messages I have the impression that it's a potpourri of changes here and changes there that are not related at all.
IMO, that's not the way PRs should be used. Instead one PR should contain only the changes for one improvement or new feature but it should try to handle 100 different issues at the same time.
They are all related for sure. But it is possible to split. I'll write a post to explain the whole thing first. Then I'll see how to split it.
Try Assembly3 (latest version 0.10.2) along with my custom build of FreeCAD at here.
And if you'd like to show your support, you can donate through patreon, liberapay, or paypal
ezzieyguywuf
Posts: 637
Joined: Tue May 19, 2015 1:11 am

Re: Code review of merged Link3 branch

Postby ezzieyguywuf » Fri Nov 22, 2019 2:43 am

wmayer wrote:
Thu Nov 21, 2019 11:18 pm
Why this again?
😮

I’m with wmayer here. Personally, I still haven’t made it past the first few commits of “the big merge” 😂 (still finalizing a pr to update the logging facilities)

I agree with the sentiment that a given PR should give a distinct incremental improvement/fix/feature.

For example, looking at your first few commits in your GroupMod branch, I wonder - what is going on? Why make extensionIsElementVisible const? Or, better put, what motivated this change? Are there other places in the code base that can also be properly labeled const? If so, why not submit that on its own as a pr? Even if it’s just the one spot.

More alarmingly: why throw an error rather than send a log when calling addDynamicProperty? I’m not too terribly familiar with this method (is it new from “the big merge”?), but this is is a drastic change from the previous behavior. I would 100% expect this to be its own PR, with an explanation behind the motivation and a discussion around if it breaks existing code.