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

[ArchWall] Do not copy shape #5222

Closed
wants to merge 6 commits into from
Closed

[ArchWall] Do not copy shape #5222

wants to merge 6 commits into from

Conversation

paullee0
Copy link
Contributor

@paullee0 paullee0 commented Dec 3, 2021

Discussions-
https://forum.freecadweb.org/viewtopic.php?style=1&t=62968&p=540585 realthunder/FreeCAD@2348994
realthunder@2348994

Thank you for creating a pull request to contribute to FreeCAD! To ease integration, we ask you to conform to the following items. Pull requests which don't satisfy all the items below might be rejected. If you are in doubt with any of the items below, don't hesitate to ask for help in the FreeCAD forum!

  • Your pull request is confined strictly to a single module. That is, all the files changed by your pull request are either in App, Base, Gui or one of the Mod subfolders. If you need to make changes in several locations, make several pull requests and wait for the first one to be merged before submitting the next ones
  • In case your pull request does more than just fixing small bugs, make sure you discussed your ideas with other developers on the FreeCAD forum
  • Your branch is rebased on latest master git pull --rebase upstream master
  • All FreeCAD unit tests are confirmed to pass by running ./bin/FreeCAD --run-test 0
  • All commit messages are well-written ex: Fixes typo in Draft Move command text
  • Your pull request is well written and has a good description, and its title starts with the module name, ex: Draft: Fixed typos
  • Commit messages include issue #<id> or fixes #<id> where <id> is the FreeCAD bug tracker issue number in case a particular commit solves or is related to an existing issue on the tracker. Ex: Draft: fix typos - fixes #0004805

And please remember to update the Wiki with the features added or changed once this PR is merged.
Note: If you don't have wiki access, then please mention your contribution on the 0.20 Changelog Forum Thread.


paullee0 referenced this pull request in realthunder/FreeCAD Dec 3, 2021
Shape.copy() will break OCCT internal shape sharing causing unnecessary
memory inflation.
@paullee0
Copy link
Contributor Author

paullee0 commented Dec 3, 2021

Why are there old commits mixed in e.g. that one on 2.1.2021 !?

Totally confused myself! See if anyone can help? Thanks

@chennes
Copy link
Member

chennes commented Dec 3, 2021

Did you have an old stash that accidentally got added? Anyway, you should be able to remove them with an interactive rebase and force push. Or we can just cherry-pick that last commit and ignore the other three.

#***************************************************************************
#* *
#* Copyright (c) 2018 - 2020 *
#* Paul Lee <paullee0@gmail.com> *
Copy link
Contributor

Choose a reason for hiding this comment

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

Please merge L4 in to L3 like so:
#* Copyright (c) 2018-2020 Paul Lee <paullee0@gmail.com> *

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks :)

@paullee0
Copy link
Contributor Author

paullee0 commented Dec 4, 2021

Did you have an old stash that accidentally got added? Anyway, you should be able to remove them with an interactive rebase and force push. Or we can just cherry-pick that last commit and ignore the other three.

Thanks ! Basically, every time I make reference to this post to do git, and know almost nothing more :-
https://forum.freecadweb.org/viewtopic.php?f=10&t=23204&start=10#p197095

I should have screwed up the local copy few weeks before during last PR. Then, I remove the whole FC folders and start over :-

  • git clone https://github.com/myGitHubId/FreeCAD.git .
  • git remote add upstream https://github.com/FreeCAD/FreeCAD.git
    Catch up my newBranch with upstream/master (the "real" master):
  • git checkout master
  • git fetch upstream
  • git merge upstream/master
    Develop:
  • git branch newBranchName
  • git checkout newBranchName
  • code, commit
  • (forget if did a - git rebase master or not)
  • git push --set-upstream origin ArchWall_realthunder_fix
    ( i.e. [ArchWall] Do not copy shape )

Not sure how I can delete the first 3 commits here...? Thanks again !

@freecadci
Copy link

pipeline status for feature branch PR_5222. Pipeline 423704155 was triggered at f620489. All CI branches and pipelines.

@luzpaz luzpaz added the WB Arch Related to the Arch Workbench label Dec 7, 2021
@chennes
Copy link
Member

chennes commented Dec 8, 2021

Is my understanding correct, that the only commit here that actually matters is f620489?

@paullee0
Copy link
Contributor Author

paullee0 commented Dec 8, 2021

Trying to follow to fix :
https://stackoverflow.com/questions/36168839/how-to-remove-commits-from-a-pull-request

  • seems delete the ArchSketchObject.py : OK
  • did i also delete the commit to ArchWall.py which should be included in this PR?

Sorry for the confusion

@chennes
Copy link
Member

chennes commented Dec 8, 2021

I think at this point it's best to stop trying to fix this PR: I've successfully cherry-picked the changes to ArchWall.py in f620489 (3 lines of code -- cherry-picking is where I pull out just the individual commit, and ignore the other stuff). Is there more that's supposed to be done? I seem to remember at some point there were more like 6-10 lines.

@chennes
Copy link
Member

chennes commented Dec 8, 2021

I pushed that one commit, anyway, it's now: ea25f69

If that's all that needed to go in, you can close this PR. If there's more, it might actually be easier for you to still close this one, delete your old branch, update your local copy, and make a new branch for it.

@paullee0
Copy link
Contributor Author

paullee0 commented Dec 8, 2021

I pushed that one commit, anyway, it's now: ea25f69

If that's all that needed to go in, you can close this PR. If there's more, it might actually be easier for you to still close this one, delete your old branch, update your local copy, and make a new branch for it.

Thanks! That's it !

Sorry for disturbing everyone. Would follow to fix my local copy.

@paullee0 paullee0 closed this Dec 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WB Arch Related to the Arch Workbench
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants