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

ticket #386 ... improve qucs/qucs/diagrams/rect3ddiagram.cpp #631

Closed
wants to merge 1 commit into from

Conversation

ldpgh
Copy link
Contributor

@ldpgh ldpgh commented Dec 13, 2016

The fix is just an improvement of the 3D data visualization. Reference for comparisioin is Qucs/version 0.0.9

Though most segments of the surface mesh are made visible there are

  • sometimes testcases have few segments missing
  • the drawn segments may differ by 1 screen pixel as found by the image comparison script img_diff.py

Screenshots has been added to the issues #386 (comment) already.

Changes in the source code (rect3ddiagram.cpp) are

  • commentlines + implementation
  • commentlines only

…to bring the hidden lines back. Test case are in qucs-test\testsuite\GUI_rect3ddiagram_prj
@guitorri guitorri changed the base branch from master to release-0.0.19 January 18, 2017 14:09
@guitorri
Copy link
Member

Thank you for the fix. I moved the base branch to the upcoming release-0.0.19. I will try to rerun Travis and test it locally.

@guitorri guitorri added this to the 0.0.19 milestone Jan 18, 2017
@guitorri
Copy link
Member

@ldpgh your patch is working fine. There are few issues you might want to fix. I can fix them for you, but I would rather let you as the author of your fix:

  • You based the changes on master, it should be rebased on release-0.0.19. That is why we have errors on Travis.

  • Your name and email don't seem to be configured on your Git settings. The commit is signed as:

Author: notallowed7 <notallowed7>  2016-12-13 15:14:25
Committer: notallowed7 <notallowed7>  2016-12-13 15:14:25
  • You signed with LDA_* your changes in the comments, that is not needed as the author becomes part of the history of changes.

  • The summary of your commit is too long. See commit rules.


Here are my suggestions:

  • Set up your email and name on your Git.

  • Make a new branch based on the latest release-0.0.19, pick your commit, clean it up, remove unnecessary comments (like he ones you signed with LDA) and send another PR. Something like:

git pull [name_of_qucs_remote]
git checkout release-0.0.19
git checkout -b branch-fix-3d-issues
git cherry-pick abcf60  # this is your commit hash
git commit --amend --author="Author Name <email@address.com>"
# editor show up, format the commit message, save and close
# remove comments signed with LDA, do another commit
# squash your second commits into the first 
git rebase -i branch-fix-3d-issues~2  
# editor show up, replace `pick` by `s` on the second commit, save and close
git push [your_fork] branch-fix-3d-issues
# create a pull request, using `release-0.0.19` as base branch

Do you have any time to do this?

@ldpgh
Copy link
Contributor Author

ldpgh commented Jan 19, 2017

@guitorri .... please apply your proposed changes. It is not clear, when I could do it.

Feel free to change what ever is required.

@guitorri
Copy link
Member

guitorri commented Jan 19, 2017

Ok. Moved your patch to #638.
Your email does not seem to be public on GitHub. Still, you are the author of the patch. Thank you.

@guitorri guitorri closed this Jan 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants