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

[Sketcher] Add copy/cut/paste sketcher commands #11537

Merged
merged 4 commits into from
Jan 22, 2024

Conversation

PaddleStroke
Copy link
Contributor

@PaddleStroke PaddleStroke commented Nov 26, 2023

Add support to copy/paste geometries in sketcher using ctrl-X/C and ctrl-V
Geometries (and relevent constraints) are copied to computer clipboard as python command text. Enabling exporting and paste in other Freecad instances or even share on forum.

Video description : https://forum.freecadweb.org/viewtopic.php?f=9&t=65892

This is a rework of #5480
So it appeared that creating commands in sketcher with the same shortcuts override CommandDoc general commands. Which solves the initial problem.

@github-actions github-actions bot added the WB Sketcher Related to the Sketcher Workbench label Nov 26, 2023
@PaddleStroke
Copy link
Contributor Author

PaddleStroke commented Nov 26, 2023

@abdullahtahiriyo there is one architectural question for the ViewProvider deleteSelected function that I use in the cut command.
This function is private. I was trying to make an attorney to access it but I am not sure how to handle it because CommandSketcherTools don't have a .h file to be included in the VP.
Then looking at this function I couldn't understand why this function would be private in the first place. Why centerSelection or updateColor is not private for example and deleteSelected is?

So I don't know if its acceptable to change it to public. If it is acceptable, then ViewProviderSketchShortcutListenerAttorney needs to be removed I guess?
If it's not acceptable, please advise where to put the attorney and what file to include in VPsketch.

@xtemp09
Copy link
Contributor

xtemp09 commented Nov 26, 2023

In LibreCAD, when user pastes from clipboard, it's automatically dragged with the mouse cursor. 1

Would it be better to mimic the same look-and-feel?

Footnotes

  1. https://youtu.be/8xeuNbu_8rc?t=23

@PaddleStroke
Copy link
Contributor Author

PaddleStroke commented Nov 26, 2023 via email

@PaddleStroke
Copy link
Contributor Author

Interesting. If the command is neither in the menu nor the toolbar, then the shortcut doesn't work.
So I added them in the menu. Now it works as expected.

Just waiting for @abdullahtahiriyo feedback about the deleteSelected function before switching to ready.

@xtemp09
Copy link
Contributor

xtemp09 commented Nov 28, 2023

Is 4000 a magic number? If it's not, then would it simpler instead of

GeoId = std::atoi(it->substr(4, 4000).c_str()) - 1;

use

GeoId = std::stoi( it->substr(4) ) - 1;

?


Would it be simpler to use compare instead of substr? In this case,

	if (it->size() > 4 && it->substr(0, 4) == "Edge")

becomes:

	if ( it->compare(0, 4, "Edge") == 0 )

formated ==> formatted


	std::string data = QGuiApplication::clipboard()->text().toStdString();
	<...>
    if (data.find("</GeometryList>", 0) != std::string::npos) {
        geoString = data.substr(0, data.find("</GeometryList>", 0) + 16);
    }

QGuiApplication::clipboard()->text() returns QString. Would it be better to use QString methods and convert into std::string in the end? Strictly speaking, you are parsing text, converting to and from QString. Would it be simpler to just use Qt XML together with other Qt features, for example, QTextStream or QString methods?

Is there a reason to use XML? Clipboard content is intended to be used on the forum, would it be better to use JSON, since it is more human-readable?


bool copySelectionToClipboard(void) returns bool. However, there is no checking in void CmdSketcherCopyClipboard::activated(int iMsg), while the function void CmdSketcherCut::activated(int iMsg) checks the return value.

void CmdSketcherCopyClipboard::activated(int iMsg) is supposed to copy to clipboard. However, if the function fails to do that, the clipboard content will stay the same. If end user copied something and then tries to copy something similar, the content of the clipboard remains unchanged. I think, it can bring confusion. Would it be better if void CmdSketcherCopyClipboard::activated(int iMsg) and void CmdSketcherCut::activated(int iMsg) show a warning in the status bar to inform the user that nothing has been copied or copied successfully?

void CmdSketcherCut::activated(int iMsg) deletes the selected items. Should it be logged?

@PaddleStroke
Copy link
Contributor Author

@xtemp09 for your 2 first comments, this was a copy paste of a code that is in many places. By the way it would probably be a good idea to make a function getting the list of geoId from the selection rather than having this block copy pasted everywhere.

XML is used because we are using the available Part::PropertyGeometryList::save(). So we are printing xml similar to what is used in the document xml. I don't really see the point of going for json since it would require writing a lot more code to parse geometries to json or to convert xml to json. And visibility is not so important, even in json a list of geometries and constraints is not going to make much sense to a reader.

void CmdSketcherCopyClipboard::activated(int iMsg) is supposed to copy to clipboard. However, if the function fails to do that, the clipboard content will stay the same.

CmdSketcherCopyClipboard::activated does not check if something got copied because this is the usual ctrlC behavior. If you try to copy an empty text in notepad for example, your clipboard remains unchanged and nothing happens. So here we do the same, if nothing is selected nothing happens.

If end user copied something and then tries to copy something similar, the content of the clipboard remains unchanged.

Sorry I don't understand.

We can add a warning in the status bar, or better a user notification if copy failed. But I am not sure this is necessary.

@xtemp09
Copy link
Contributor

xtemp09 commented Nov 29, 2023

There is no reason for the function to return value if this value is not taken into consideration at all. If the value is somewhat important, should it be checked and some action performed?


About the last part. If a user copies similar fragments twice, but the last time the program failed to copy, the clipboard buffer will contain the first fragment. I just thought that the user should be notified about the failure to avoid copy-and-paste mistakes.

@PaddleStroke
Copy link
Contributor Author

@xtemp09 thanks for the review. I will check them in more details when I get back to this branch.

@abdullahtahiriyo
Copy link
Contributor

There are several ways to copy and paste between sketches or even inside one. One option is XML. Another option is using commands (via PythonConverter).

The XML used in save function stores a geometry and all its extensions, so as to be able to retrieve it "as is". One caveat is that geometry extensions may have geometry specific information (specific to one sketch, to one specific use of geometry as people in ArchSketch WB are using by storing in extensions meta-information, or even just a Geometry ID).

While using XML is at a first glance a brilliant idea (close to zero effort implementation), the question arises how to handle the issue of copying what is wanted, but not copying what is not wanted. Taking into account that part of it may come from stock FreeCAD by design (such as the Geometry ID or geometry index), but others are even inserted by third party tools. Deciding what to copy "as is" and what to "regenerate" (such as unique indices/uuids) or not copy at all is not straight-forward. We should also put an eye to impact for external geometry (ExternalGeometryExtension) and its use by code coming from the TopoNaming project (at least RT's implementation used it).

On the other hand we have PythonConverter, which copies console commands to reproduce the geometry, but will not handle any other special extension information. It is like adding the elements anew. The best advantage I see to this path is that the commands can be copied anywhere and even used to create macros. It is the "internal macro language" used by FreeCAD. The drawbacks are that some other information from extensions, which might perhaps be desirable to copy (visualisation attributes, or layer grouping information when it is available), is, at least in the current state of development of PythonConverter ignored.

So XML is copy all and then go and figure out what needs to be removed. While PythonConverter is copy the basic and add whatever else you may need.

This is one of the reasons (scarce review time being the other) why this PR (or better said its predecessor) has been queuing for some time.

So essentially I say two things:
(a) we need assess very well the impact of this and select the right choice (XML or PythonConverter),
(b) I will only be able to take a look to this next year, after merging all pending DSH staff and looking to Ajinkya's general tangency constraint code, which is a rather complex topic with potential very high impact if we do not get it right.

@PaddleStroke
Copy link
Contributor Author

I think python converter is a great idea. This way the code can be used in macros as well. I didn't thought about using this, and at the time of the initial work, python converter was not available.

@PaddleStroke
Copy link
Contributor Author

Yes next year is fine no hurry.

@abdullahtahiriyo
Copy link
Contributor

Indeed. Pythonconverter was not available at the time, and I still think your idea of using XML code for serialisation is brilliant in many ways. Very creative approach.

Anyway, we look at it next year.

@PaddleStroke
Copy link
Contributor Author

I have move from using XML to use python instead. This way the copied text can be used in macros and so on.

Anyone willing to test and review?

@xtemp09
Copy link
Contributor

xtemp09 commented Jan 11, 2024

I've just tried it. Seems to be working. When pasting FreeCAD reports sometimes:

02:16:10  Sketcher constraint number 9 is malformed!
02:16:10  Unnamed#Sketch001: The Sketch has malformed constraints!
02:16:10  Sketcher constraint number 9 is malformed!
02:16:10  Sketcher constraint number 9 is malformed!
02:16:10  Unnamed#Sketch001: The Sketch has malformed constraints!
The clipboard:
# Copied from sketcher.
lastGeoId = len(ActiveSketch.Geometry)

geoList = []

geoList.append(Part.LineSegment(App.Vector(-51.587891,39.518017,0.000000),App.Vector(-51.587891,10.801011,0.000000)))

geoList.append(Part.LineSegment(App.Vector(-51.587891,10.801011,0.000000),App.Vector(24.187309,10.801011,0.000000)))

geoList.append(Part.LineSegment(App.Vector(24.187309,10.801011,0.000000),App.Vector(24.187309,39.518017,0.000000)))

geoList.append(Part.LineSegment(App.Vector(24.187309,39.518017,0.000000),App.Vector(-51.587891,39.518017,0.000000)))

geoList.append(Part.Circle(App.Vector(0.000000, 0.000000, 0.000000), App.Vector(0.000000, 0.000000, 1.000000), 11.719850))

objectStr.addGeometry(geoList,False)
del geoList
constraintList = []
constraintList.append(Sketcher.Constraint('Coincident',  0, 2,  1, 1))
constraintList.append(Sketcher.Constraint('Coincident',  1, 2,  2, 1))
constraintList.append(Sketcher.Constraint('Coincident',  2, 2,  3, 1))
constraintList.append(Sketcher.Constraint('Coincident',  3, 2,  0, 1))
constraintList.append(Sketcher.Constraint('Vertical',  0))
constraintList.append(Sketcher.Constraint('Vertical',  2))
constraintList.append(Sketcher.Constraint('Horizontal',  1))
constraintList.append(Sketcher.Constraint('Horizontal',  3))
constraintList.append(Sketcher.Constraint('Coincident',  0, 3,  -1, 1))
objectStr.addConstraint(constraintList)
del constraintList
OS: Ubuntu 22.04.3 LTS (KDE/plasma)
Word size of FreeCAD: 64-bit
Version: 0.22.0dev.35569 (Git)
Build type: Debug
Branch: copy_paste_2023
Hash: cf3aeb6618cf68af754a74ac97f094fbb8188702
Python 3.10.12, Qt 5.15.12, Coin 4.0.0, Vtk 9.1.0, OCC 7.5.1
Locale: English/United States (en_US)

Is there a reason to use boost::format instead of std::format? boost::format is supposed to be faster, but in the strings like this:

return boost::str(boost::format("Sketcher.Constraint('Parallel', %s %i, %s %i)")
                                   % lastGeoId % constr->First % lastGeoId % constr->Second);

there is no gain in speed.

@PaddleStroke
Copy link
Contributor Author

PaddleStroke commented Jan 11, 2024 via email

@PaddleStroke PaddleStroke force-pushed the copy_paste_2023 branch 2 times, most recently from 53c695e to 66c728b Compare January 12, 2024 09:10
@PaddleStroke
Copy link
Contributor Author

PaddleStroke commented Jan 12, 2024

I have refactored my changes to python converter, it is cleaner now. Also I tested all constraint types (I think) and added the missing angleByPoint in pythonconverter.

Regarding the boost::format vs std::format I have no idea about it. As this is code that Abdullah wrote I don't want to change it because he may have had a reason and it is not related to this PR.

@xtemp09
Copy link
Contributor

xtemp09 commented Jan 12, 2024

The copy/paste seems to be working fine and looks pretty handy.


When compiling, compiler warns a lot about unused parameters. Lint shows these warnings, like

unused parameter 'geoId3' [-Wunused-parameter]

Is there a reason to separate entries with empty line when filling geoList and constrGeoList? You don't do this when filling constraintList.

I think it would be neater if there was an empty line (or two) between the fragments of constrGeoList, geoList and constraintList, while empty lines between the entries was absent.

@PaddleStroke
Copy link
Contributor Author

PaddleStroke commented Jan 16, 2024

Thanks for your comments.
I have :

  • corrected the lint warnings.
  • adjusted the printed text. Now it looks like :

#Copied from sketcher. From:
#objectStr = App.getDocument('offset_tests').getObject('Sketch002')
lastGeoId = len(ActiveSketch.Geometry)

geoList = []
geoList.append(Part.LineSegment(App.Vector(-1392.965698,733.026794,0.000000),App.Vector(-926.936890,1448.002441,0.000000)))
geoList.append(Part.LineSegment(App.Vector(-926.936890,1448.002441,0.000000),App.Vector(-576.419617,733.026794,0.000000)))
geoList.append(Part.LineSegment(App.Vector(-576.419617,733.026794,0.000000),App.Vector(-1392.965698,733.026794,0.000000)))
geoList.append(Part.LineSegment(App.Vector(-2524.101074,1411.982788,0.000000),App.Vector(-1863.643188,2094.213379,0.000000)))
geoList.append(Part.LineSegment(App.Vector(-1863.643188,2094.213379,0.000000),App.Vector(-1783.807739,1223.280518,0.000000)))
geoList.append(Part.LineSegment(App.Vector(-1783.807739,1223.280518,0.000000),App.Vector(-2524.101074,1411.982788,0.000000)))
geoList.append(Part.LineSegment(App.Vector(-909.245911,1905.511475,0.000000),App.Vector(-187.097076,1724.067017,0.000000)))
geoList.append(Part.LineSegment(App.Vector(-187.097076,1724.067017,0.000000),App.Vector(-304.932681,2270.133664,0.000000)))
geoList.append(Part.LineSegment(App.Vector(-445.697261,2316.250733,0.000000),App.Vector(-909.245911,1905.511475,0.000000)))
geoList.append(Part.ArcOfCircle(Part.Circle(App.Vector(-388.798402, 2252.036298, 0.000000), App.Vector(0.000000, 0.000000, 1.000000), 85.796118), 0.212531, 2.295865))
objectStr.addGeometry(geoList,False)
del geoList

constraintList = []
constraintList.append(Sketcher.Constraint('Coincident', lastGeoId + 0, 2, lastGeoId + 1, 1))
constraintList.append(Sketcher.Constraint('Coincident', lastGeoId + 1, 2, lastGeoId + 2, 1))
constraintList.append(Sketcher.Constraint('Coincident', lastGeoId + 2, 2, lastGeoId + 0, 1))
constraintList.append(Sketcher.Constraint('Horizontal', lastGeoId + 2))
constraintList.append(Sketcher.Constraint('Coincident', lastGeoId + 3, 2, lastGeoId + 4, 1))
constraintList.append(Sketcher.Constraint('Coincident', lastGeoId + 4, 2, lastGeoId + 5, 1))
constraintList.append(Sketcher.Constraint('Coincident', lastGeoId + 5, 2, lastGeoId + 3, 1))
constraintList.append(Sketcher.Constraint('Coincident', lastGeoId + 6, 2, lastGeoId + 7, 1))
constraintList.append(Sketcher.Constraint('Coincident', lastGeoId + 8, 2, lastGeoId + 6, 1))
constraintList.append(Sketcher.Constraint('Tangent', lastGeoId + 8, 1, lastGeoId + 9, 2))
constraintList.append(Sketcher.Constraint('Tangent', lastGeoId + 7, 2, lastGeoId + 9, 1))
objectStr.addConstraint(constraintList)
del constraintList

@xtemp09
Copy link
Contributor

xtemp09 commented Jan 16, 2024

Strictly speaking, I would beatify it even more, added spaces to some App.Vectors.

geoList.append(Part.LineSegment(App.Vector(-1392.965698,733.026794,0.000000),App.Vector(-926.936890,1448.002441,0.000000)))

is spaceless while

Part.Circle(App.Vector(-388.798402, 2252.036298, 0.000000)

contains spaces.

I would also add additional check, if a number contained zeroes, I would delete them, therefore:

geoList.append(Part.LineSegment(App.Vector(-1392.965698,733.026794,0.000000),App.Vector(-926.936890,1448.002441,0.000000)))

would become:

geoList.append(Part.LineSegment(App.Vector(-1392.965698, 733.026794, 0.0), App.Vector(-926.936890, 1448.002441, 0.0)))

and

App.Vector(0.000000, 0.000000, 1.000000)

would become:

App.Vector(0.0, 0.0, 1.0)

or, if possible,

App.Vector(0, 0, 1)

This is more readable for human eye.

But, it's just my opinion.

@PaddleStroke
Copy link
Contributor Author

I fixed the white space missing in some geos.
Regarding removing the excess 0, I'm not sure how to do it cleanly, I don't want to risk messing things up with it, so if required that will be a future PR.

@PaddleStroke PaddleStroke marked this pull request as ready for review January 16, 2024 17:44
@PaddleStroke
Copy link
Contributor Author

@chennes this PR is ready if you're looking for something to review ;)

@chennes chennes merged commit 6dfbdfc into FreeCAD:main Jan 22, 2024
9 checks passed
@maxwxyz
Copy link
Collaborator

maxwxyz commented Jan 23, 2024

@PaddleStroke should I add them in the Sketcher context menu if we have a selection? I have to adjust it to also work with external geometry as well.

@Roy-043
Copy link
Contributor

Roy-043 commented Jan 30, 2024

Unless I am mistaken (or have missed an intermediate PR) there is something wrong with how constraints are pasted. Their geoids are not updated.

  1. Open the attached file (remove the .zip extension).
  2. Open the sketch.
  3. Box-select all elements and use Ctrl+C.
  4. Move all endpoints to a new position.
  5. Use Ctrl+V.
  6. Result: the pasted symmetry constraint is applied to an existing element.

paste-geoids-test.FCStd.zip

OS: Windows 8 build 9600
Word size of FreeCAD: 64-bit
Version: 0.22.0dev.35720 (Git)
Build type: Release
Branch: main
Hash: 00cd50bbf49c810555899efe112ec7595c76cd3a
Python 3.10.13, Qt 5.15.8, Coin 4.0.2, Vtk 9.2.6, OCC 7.6.3
Locale: C/Default (C) [ OS: Dutch/Netherlands (nl_NL) ]
Installed mods:

@PaddleStroke
Copy link
Contributor Author

It should update the geoId, I may have broken things with my latest commit dealing with the boolean blindness.

@Roy-043
Copy link
Contributor

Roy-043 commented Jan 30, 2024

Hmm, you are right, it does for most constraints. But apparently the symmetry constrain, which I decided to use for my first tests, is not handled properly.

@PaddleStroke
Copy link
Contributor Author

I have just tested on my side with a single line with a length constraint. And it fails as you described. I will check in more details now

@PaddleStroke
Copy link
Contributor Author

I found the issue, it was not updating geoId when only one geometry was selected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WB Sketcher Related to the Sketcher Workbench
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants