[bug] Cant add reference constraint to already defined circle/arc

[openBrain]

Pull request
I implemented the 1bis solution explained 2 posts above.
When coding, I see some other things that may eventually be discussed. Will come back on this later.

[kisolre]

Will come back on this later.

Any updates on this?

[openBrain]

Any updates on this?

PR is not merged yet so I think better not to speak yet of somewhat minor things compared to the current bug.

[abdullah]

Any updates on this?

PR is not merged yet so I think better not to speak yet of somewhat minor things compared to the current bug.

I will try to look into it during this week.

I have seen the code in GitHub and it changes the behaviour, so I would like to test that this is not bringing undesirable behaviour.

[openBrain]

I will try to look into it during this week.

I have seen the code in GitHub and it changes the behaviour, so I would like to test that this is not bringing undesirable behaviour.

Yep, I implemented option 1bis described here :

Actually behavior is slightly changed but nothing bad IMO.
Once PR is merged, more users can test and we can fine-tune specific behaviors in case of multi-selection, normal/reference/external geometry mix, ...
But I prefer baby steps. Current PR is a minimal fix for bug declared in OP.

Tell me if rebasing is needed.

[abdullah]

Yep, I implemented option 1bis described here :

But I prefer baby steps. Current PR is a minimal fix for bug declared in OP.

Ok. I see. So:
1 - Addition of multiple circles radii (of different radius values) at once is removed.
2 - All radii are always made equal with multiple selection, even when reference is chosen in the dialog (which now actually makes it reference,this is the bug fixed).
3- It is implemented for radius, not yet for diameter.

Obs1.gif (120.68 KiB) Viewed 1028 times

I will take give a run.

[abdullah]

Ok. I have merged it.

I was initially reluctant to lose the ability to simultaneously assign different radii at once (as it changes what is expected behaviour before this commit).However:

1. I think it is safe to assume that 99% per cent of the time multiple selection + radius is used, the intention is to have equally sized circles. So 99% of the time having to click yes in the pop up is saved with the commit.

2. When reference may be used, it appears that in 99% of the cases, the idea is to have equally sized circles that can be dragged to a new size and the the reference made driving.

3. If different radii are actually wanted, it is unlikely that the physical dimensions before constraining match the actual wanted value. A single edit value is of little use in this scenario. It wiould require to go editing each value one by one afterwards. For this case, no implementation of the select-apply constraint method seems to be satisfactory. The right tool appears to be the select radius constraint, go clicking each circle (and set the value right after each click).

4. On an ancillary basis, the commit simplifies the code and makes it uniform with the rest of dimensions, which improves maintainability.

Hopefully I did not miss anything.

If this settles well, we would need the same brought to the diameter constraint for consistency.

Thanks for the contribution!

[openBrain]

Ok. I have merged it.

Great, thanks.

I was initially reluctant to lose the ability to simultaneously assign different radii at once (as it changes what is expected behaviour before this commit).However:

1....

2....

3....

That was exactly my thought when I explained and proposed in the following reply.

But you said it in a much more verbose and clear way.

4. On an ancillary basis, the commit simplifies the code and makes it uniform with the rest of dimensions, which improves maintainability.

Also in 100% of use cases, it avoids to get a message box to pop up where you have to click, which was losing some efficiency.

Hopefully I did not miss anything.

Hey, if we want each commit of the dev branch to bring a perfect software, I see only one solution : stop coding. At least I have to.

If this settles well, we would need the same brought to the diameter constraint for consistency.

Saw the title of my PR? I can't lie, I missed it doesn't fix diameter as well. Will give it a go
Having a greenhouse version of FreeCAD would really be cool to have a quick user testing...

Thanks for the contribution!

Sure? Introducing this bug was a former contribution from myself.

[abdullah]

But you said it in a much more verbose and clear way.

You know I am verbose.

While it might seem I love the sound of my own voice, I do it for two totally different reasons. First, because I like transparency when I make decisions. I think it is best that the community knows why I decide for merging something or against. Second, because I cannot rely on my own memory and when problems arise, I like to read why on earth I decided to merge something. It also helps me improve.

Also in 100% of use cases, it avoids to get a message box to pop up where you have to click, which was losing some efficiency.

That is already in the last sentence of 1 in my verbose version

Hey, if we want each commit of the dev branch to bring a perfect software, I see only one solution : stop coding. At least I have to.

There is no such thing as perfect software, as much as there is no perfect executions of a musical piece. There are very good software implementations and musical piece executions. FreeCAD is not perfect, but by development branch standards, is rather good. As everything is life, it comes at a cost.

I am unhappy with myself that it took so long for me to merge this PR. However, my comment is not to be construed as a critic to the code or the functionality, rather it reflects my lack of own confidence when reviewing a commit after several months without coding and reviewing. This generally auto-corrects as I start coding and reviewing again.

If this settles well, we would need the same brought to the diameter constraint for consistency.
Saw the title of my PR? I can't lie, I missed it doesn't fix diameter as well. Will give it a go
Having a greenhouse version of FreeCAD would really be cool to have a quick user testing...

That's all right. I had the tool locked to diameter (you know this "new" feature that remembers what you last used, radius or diameter). No surprise I could not see the bug fixed. Baby steps is ok.

Now that I know why you were removing the old behaviour and inverting the order of the equality and radius constraints, merging the diameter one should be faster.

I am not sure about a greenhouse, as it requires more maintenance work and eventually there will be some distribution packager that will package the greenhouse version, making a mess for the end user which no longer knows which version he is using.

But I think we should have automatic builds of PRs for testing. We are already running PRs through CI. Even if it is just on one platform.

Thanks for the contribution!

Sure? Introducing this bug was a former contribution from myself.

Yes and no.

You introduced the reference checkbox which worked well on constraints using the common code. You did not realise about these special implementation, neither did the reviewer, which most probably was me.

I thank you for fixing you own bug (not everybody does it), but I also thank you for the rest of the contribution.

[chrisb]

1. I think it is safe to assume that 99% per cent of the time multiple selection + radius is used, the intention is to have equally sized circles. So 99% of the time having to click yes in the pop up is saved with the commit.

2. When reference may be used, it appears that in 99% of the cases, the idea is to have equally sized circles that can be dragged to a new size and the the reference made driving.

I fully agree, except that my guess would be at least 1% higher making it 99.9%