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

[Core] Create ElementMap.h and tests #8923

Merged
merged 2 commits into from
Mar 23, 2023

Conversation

AjinkyaDahale
Copy link
Contributor

ElementMap.h for now contains a list of constants which will be used in the ElementMap class in a future phase of toponaming solution.

Intended to fix #8768.

@github-actions github-actions bot added the Core Issue or PR touches core sections (App, Gui, Base) of FreeCAD label Mar 17, 2023
@AjinkyaDahale
Copy link
Contributor Author

As mentioned in a comment in the issue, I have not moved constants from TopoShapeOpCode since it did seem to belong to Part-specific items.

@AjinkyaDahale
Copy link
Contributor Author

@realthunder are there any other constants that can be stored here? Also, question, where are the mod/gen/upper/lower strings in your own branch?

@JohnAD
Copy link
Contributor

JohnAD commented Mar 18, 2023

@AjinkyaDahale

My thinking of moving the TopoShapeOpCode items was that future programmers would find it convenient to have a single place to "interpret the meaning" of a particular mapped name. Since since these are simple constants from a standard default workbench, it would not add much complication to the Part workbench later.

Is the new TopoShapeOpCode.h file more about the naming algorithm or more about the Part workbench? It is both; so there is not a clear answer for where to put them.

If you are keeping them separate, you might consider placing the TopoShapeOpCode.h into the Part workbench now, and place a comment reference in the ElementMap.h file to the TopoShapeOpCode for future programmers to follow.

@AjinkyaDahale
Copy link
Contributor Author

Is there a reason to store constants as char* instead of std::string or anything else?

@chennes
Copy link
Member

chennes commented Mar 20, 2023

src/App/ElementMap.h Outdated Show resolved Hide resolved
const std::string POSTFIX_MOD = ";:M";
const std::string POSTFIX_GEN = ";:G";
const std::string POSTFIX_MODGEN = ";:MG";
const std::string POSTFIX_DUPLICATE = ";D";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this the only one that doesn't have a :? It looks odd. And for that matter, why do all the others have a : anyways?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the answer to the first is there may be other meanings when the : is not used. As for why this particular postfix doesn't have the colon, I'm just taking it from the issue. I don't see it used in the development/toponaming branch, but RT may have just not merged it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JohnAD @realthunder could you pitch in?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My intention is for ;: as the separator for a postfix that may be used for parsing later on. ;D is not exposed in my original code as a postfix. It is internally used as a default way to rename duplicated element that may be overridden by derived class.

@adrianinsaval
Copy link
Member

TopoShapeOpCode.h has mostly Part WB specific stuff but I also see some mentions of sketcher and draft, presumably when implementing the algorithm on other WBs one would want to add the op codes for those there too so unless we're going to split those for each workbench I think it might make more sense to move it into App

Note that I'm very much an amateur so don't take me too seriously.

@AjinkyaDahale
Copy link
Contributor Author

TopoShapeOpCode.h has mostly Part WB specific stuff but I also see some mentions of sketcher and draft,...

AFAIK Part is used in those WB's as well.

I do expect there may be other such WB specific names. Can't think of operations right now, but I'm sure there may be operations that are defined for, say path, but not used in part.

@freecadci
Copy link

pipeline status for feature branch PR_8923. Pipeline 812852228 was triggered at e3ced6c. All CI branches and pipelines.

@freecadci
Copy link

pipeline status for feature branch PR_8923. Pipeline 813228988 was triggered at 5fdde9d. All CI branches and pipelines.

@AjinkyaDahale
Copy link
Contributor Author

See https://github.com/KDE/clazy/blob/master/docs/checks/README-non-pod-global-static.md

@chennes I've replaced the std::string global vars with char* now.

@AjinkyaDahale AjinkyaDahale marked this pull request as ready for review March 21, 2023 16:18
@chennes
Copy link
Member

chennes commented Mar 21, 2023

Awesome. I'm thinking we'll hold the merge for a short time here to see if either @realthunder or @JohnAD can comment on the : syntax question above. That feels like something we want to make sure we understand before going too much further. When you have a minute can you take a look at #8974? There wasn't a lot to do there, so it should be a fast review.

@freecadci
Copy link

pipeline status for feature branch PR_8923. Pipeline 813587061 was triggered at 125eacd. All CI branches and pipelines.

@realthunder
Copy link
Collaborator

The first priority of the topo naming encoding scheme is to generate unique and stable names for geometry elements. Decoding and history tracing is a good to have feature, which is why there is no strict syntax definition. Another reason is that, for many cases, there are ambiguities when doing history tracing. You can't get back the source element even with a strict syntax definition. What we should guarantee is that a model done by an old version syntax will work correctly and can continue modeling with new syntax in newer release. The syntax change may break history tracing, which may affect how good the software can auto fix common topological changing problem. But that's acceptable, as long as it can detect the change and report the failure to user in a meaningful way.

With that in mind, you'll see that TopoShapeOpCodes is just a convenience to centralize all the model operation code. It's exact content is not that important. I prefer to make it in Part workbench, because currently only Part and other workbenches that uses Part make use of it. It is good not to have duplicate op codes, but not a must. So spread the codes in other workbenches shouldn't be a problem either.

@freecadci
Copy link

pipeline status for feature branch PR_8923. Pipeline 813945472 was triggered at f0e9d86. All CI branches and pipelines.

Test is a dummy for now. `ElementMap.h` only contains const values.
@freecadci
Copy link

pipeline status for feature branch PR_8923. Pipeline 814738102 was triggered at 2aca160. All CI branches and pipelines.

Constants to be used in the future.

Apply suggestions from code review

Co-authored-by: Chris Hennes <chennes@pioneerlibrarysystem.org>
@freecadci
Copy link

pipeline status for feature branch PR_8923. Pipeline 814755132 was triggered at 27a9879. All CI branches and pipelines.

@chennes
Copy link
Member

chennes commented Mar 23, 2023

@AjinkyaDahale this looks ready to merge to me -- anything else you want to do before that happens?

@AjinkyaDahale
Copy link
Contributor Author

@chennes no I think we're fine.

@chennes chennes merged commit c32ee26 into FreeCAD:master Mar 23, 2023
@sliptonic sliptonic deleted the toponaming-p1-4 branch May 13, 2023 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core Issue or PR touches core sections (App, Gui, Base) of FreeCAD Toponaming
Projects
Development

Successfully merging this pull request may close these issues.

[Issue] TopoNaming Phase 1 (card 4 of 4) - ElementMap Header file
7 participants