Core datums : Card2 : Core implementation #18126
Conversation
PaddleStroke
force-pushed
the
core_LCS2
branch
from
64302ea
to
e47b811
Compare
|
@chennes this one is the big part for Core. After this merges I can prepare second part for Part module. |
github-actions
bot
added
Mod: Core
|
Comments from #16675 apply here as well. Also that svg change from Card1 should be probably dropped from here. I can fix that, if there is a will to take changes back. |
|
@PaddleStroke, here is this PR cleaned up: https://github.com/3x380V/FreeCAD/tree/core_LCS2_ |
|
@3x380V you mean this card2 or the initial PR? In your link it is 36 commits so it looks like the initial commit |
This card2. You need to pull your main as I rebased it against latest and greatest. Alternatively you can see here individual commits: https://github.com/3x380V/FreeCAD/commits/core_LCS2_/ and I guess you can fetch them from there. |
Changing rotation in |
There is no advantage. Review is per commit (doing single well defined thing), so there is no limit on commit count. However each commit have to build and pass tests. Failing to comply with this rule causes trouble when bisecting. No need to worry about that only in case everyone does perfect job, so there is no need to hunt bugs by bisecting.
Indeed. However in this particular case it revealed breaking above rule. Being random contributor without any influence on the process I opted to repeat this argument over and over until someone responsible either get that into his head or provide counterargument ;-) |
|
You are right. I'm working on the commits then to untangle all of it. |
PaddleStroke
force-pushed
the
core_LCS2
branch
2 times, most recently
from
8eeaf5a
to
f82c9b8
Compare
|
Commits cleaned. This is now ready @chennes |
|
@PaddleStroke, I beg to differ.You ignored my cleanups :) I started fixing that after your force-push, but... Are you willing to use them? |
|
Can you please tell me specifically what you did? I wasn't sure how to compare. And there were some things to fix in the commits to make all commits build. |
PaddleStroke
force-pushed
the
core_LCS2
branch
from
f82c9b8
to
d47ddbe
Compare
|
I made sure clang-format will left your work untouched after enabled on App, fixed some else after returns, optimized few functions. |
|
@3x380V ok thanks. Then please let me know the fixes you do and how I can apply them (Consider that I'm a bit of a noob in git, I mostly know rebasing and cherry-picking). |
|
@PaddleStroke, I cannot attach patch (this is known issue), so you need a little work (or you can donwload it as text file core_LCS2.txt) Most changes are to make |
That certainly sounds like a problem. Can you point it to me so I can check there's nothing amiss?
I moved those. Then there's clang-format which I have not done. And which is not perfect as a result of the rebase. |
That's why I'm proposing you just replace commits with those ready to go and save yourself great amount of time. I had to resolve many conflicts before getting there, so there is no point to repeat it yourself. |
PaddleStroke
force-pushed
the
core_LCS2
branch
from
d47ddbe
to
d365d66
Compare
|
@3x380V I have removed my commits, then merged your pull request to core_LCS2. So the 5 commits here are now yours. Can you please double check? Btw did you compile your version and checked if it was correct? |
|
|
|
Ok. I'm compiling to check if everything still works as it should. Then it's ready to go in right @3x380V ? |
|
Yes. Otherwise I cannot read diffs carefully enough and need holidays :) |
|
|
||
| DatumElement::DatumElement() | ||
| DatumElement::DatumElement(bool hideRole) |
There was a problem hiding this comment.
Do we really need to change this? It's a single argument it's not like it's problematic. I don't really understand this boolean blindness anyway. Unless someone is editing source code with notepad, the IDE let you see the signature of the function easily enough.
I think rejecting any use of boolean is a bit drastic. Searching for "(bool " in the source code gives 1510 results. If for each of those function we implemented a enum class, imagine the mess that would be.
If you still want me to take action please advise your prefered replacement solution. Thanks
There was a problem hiding this comment.
@PaddleStroke, when editing your commits, please run clang-format -i src/App/whatever.cpp for files in App directory to be sure they still conform.
(as someone who is editing code with Vim, I'm not big fan of declaring enum for sole purpose of boolean blindness idiom)
There was a problem hiding this comment.
All of these review suggestions are that: suggestions. @wwmayer first introduced me to the modern notion of "boolean blindness", and at the call site I do find it is easier to read calls that don't have booleans in them. With just a single argument the benefit of switching to an enumeration is marginal (at best), but I am happy to defer to @wwmayer here about whether in these cases we should indeed require a new enumeration.
There was a problem hiding this comment.
I agree that making an enum just to replace a boolean parameter is not ideal. Better to look at the root cause of the problem: the boolean parameter is causing the functions to do two different things (two responsibilities) -- maybe there actually should be two functions.
There was a problem hiding this comment.
All of these review suggestions are that: suggestions
How does one tell the difference between a "suggestion" and something that will prevent merge?
There was a problem hiding this comment.
After thinking about it, I think this boolean can be removed and replaced by a function 'hideRole'. Then this function can be called after the initialization.
However as this boolean is not used in this PR (it will be in next PR implementing the Part subclasses of those datums) I ask that we leave this as it is now as to not create more conflicts. And I'll replace this bool in this next PR.
Most of App::Origin is moved into this sub class of App::Origin. Add App::Point. Change graphics of the planes/axis. Remove scale-by-content behavior and make it fixed size on screen.
PaddleStroke
force-pushed
the
core_LCS2
branch
from
4e0e3a8
to
b4762e8
Compare
|
Build and confirm it works as it should. The style of the origin objects is changed and the point is added. That's all. |
|
@PaddleStroke, glad it worked. Do you want any help with the rest of changes once this PR is sorted out? |
|
I think the rest should be fine. The commit history is better and I think there won't be any conflicts. So I think it will be ok. I'll let you know if I struggle ;) |
|
How does just added Assembly: Simulation implementation relate to this PR? Was it in intentional? |
|
It was a mistake. I dropped it and force pushed but the commit is still here in this PR. I do not know why. Edit: rewording last commit and pushing again solved the issue. |
PaddleStroke
force-pushed
the
core_LCS2
branch
from
b4762e8
to
b7f1a0c
Compare
|
@wwmayer could you please tell if you intend to check this at some point? A 'no' or 'yes later' are perfectly acceptable and would be better than silence so that we know and don't wait for nothing. Thanks. |
Part 2 of #16675
These are the changes to Core :
Note : no UI to create the new LCS objects. Only visible change is the origin appearance.
Note 2 : NO fix of the X axis orientation. So no migration script here.
Fix #16966
fix #17199
fix #9391
fix #15746
Thanks @3x380V for rebasing and fixing the commits!