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

Annotation text segment extending #14

Merged
merged 5 commits into from
Jan 31, 2022
Merged

Annotation text segment extending #14

merged 5 commits into from
Jan 31, 2022

Conversation

H1manshu21
Copy link
Contributor

This is a patch to extend the text segment in the annotation primitive to have text size and text rotation angle parameters.
Reference to https://sourceforge.net/p/brlcad/patches/535/

@drossberg
Copy link
Member

I think, you used the wrong functions for serialization. The new variables are doubles, therefore bu_cv_htond()/bu_cv_ntohd() should be used.

@brlcad
Copy link
Member

brlcad commented Jan 20, 2022

@Himanshu40 great job keeping this PR up to date, but it appears as though you're doing git pull's from main which generates these merge commits. Running "git pull --rebase" avoids that, but can you see if you can delete the merge commits? This looks relevant: https://stackoverflow.com/questions/33502205/how-to-delete-merge-commits

@H1manshu21
Copy link
Contributor Author

H1manshu21 commented Jan 21, 2022

@Himanshu40 great job keeping this PR up to date, but it appears as though you're doing git pull's from main which generates these merge commits. Running "git pull --rebase" avoids that, but can you see if you can delete the merge commits? This looks relevant: https://stackoverflow.com/questions/33502205/how-to-delete-merge-commits

Wow, I never know that. Thanks for sharing on how to rebase. In future PRs, I will keep them in mind.

@brlcad
Copy link
Member

brlcad commented Jan 21, 2022

@Himanshu40 great job keeping this PR up to date, but it appears as though you're doing git pull's from main which generates these merge commits. Running "git pull --rebase" avoids that, but can you see if you can delete the merge commits? This looks relevant: https://stackoverflow.com/questions/33502205/how-to-delete-merge-commits

Wow, I never know that. Thanks for sharing on how to rebase. In future PRs, I will keep them in mind.

It's actually pretty useful to always rebase when pulling so local commits are pushed forward in the chain instead of resulting in merge commits. You can set it up to work that way by default with: git config pull.rebase true

That will make all "git pull" be a "git pull --rebase" unless the branch was specifically configured with rebase false, iirc.

src/librt/primitives/annot/annot.c Outdated Show resolved Hide resolved
@brlcad
Copy link
Member

brlcad commented Jan 21, 2022

Note, you really only want to force-push whenever you absolutely have to (such as deleting merge commits). Otherwise, it looses all associativity on github and in any checkouts people already pulled.

@H1manshu21
Copy link
Contributor Author

Note, you really only want to force-push whenever you absolutely have to (such as deleting merge commits). Otherwise, it looses all associativity on github and in any checkouts people already pulled.

I came to know that merging creates a commit while rebasing doesn't. So I should not do the push until I create the next commit for PR. So I should not do force push always after I rebase. I should push only when I am about to do commit.

@brlcad
Copy link
Member

brlcad commented Jan 22, 2022

I came to know that merging creates a commit while rebasing doesn't. So I should not do the push until I create the next commit for PR. So I should not do force push always after I rebase. I should push only when I am about to do commit.

You can (no force) push as many times as you want to a PR branch, usually without issue. It is only an issue when you’re trying to push merge commits. If you had a bigger PR with a hundred commits to review, even a couple merge commits wouldn’t be an issue. When it’s a nice small neat PR like yours that it matters more.

That’s why if you always do git pull --rebase, it won’t make merge commits so you can continue editing, committing, and pushing to update the PR. The force push was just a one-time thing because you’d already pushed the merge commits and you have to force to undo them. That’s almost never done, this was a very rare exception.

@H1manshu21
Copy link
Contributor Author

H1manshu21 commented Jan 22, 2022

I have again forced pushed the commits and I learned the mistakes. I will keep it in mind next time. I am just testing it out after rebase if I can update my test branch with the main branch but didn't work as this PR listed out all the commits from the main branch. Looks like a local branch can be merged into remote, but as git push performs only fast-forward merges so it throws an error. So remote branch can't be fast-forwarded to local?

@H1manshu21
Copy link
Contributor Author

There is some issue with the changes I have done.

  1. When I open a .g file that already contains some annots and try to draw annots on screen but it can't draw. Sometimes it is able to draw and sometimes it doesn't.
  2. If I create a .g file and start creating an annot using in then mged is able to show in the window.

@H1manshu21
Copy link
Contributor Author

Some of the .g files.
annotTest.zip

@drossberg
Copy link
Member

There is some issue with the changes I have done.

1. When I open a .g file that already contains some annots and try to draw annots on screen but it can't draw. Sometimes it is able to draw and sometimes it doesn't.

2. If I create a .g file and start creating an annot using `in` then mged is able to show in the window.

If you load a file with bu_cv_ntohd() (the new version) which was written with htonl() (the old version), you get an error. But this shouldn't be a problem, because htonl() was in your copy of the brlcad repository only. It didn't made it into the official version.

@H1manshu21
Copy link
Contributor Author

There is some issue with the changes I have done.

1. When I open a .g file that already contains some annots and try to draw annots on screen but it can't draw. Sometimes it is able to draw and sometimes it doesn't.

2. If I create a .g file and start creating an annot using `in` then mged is able to show in the window.

If you load a file with bu_cv_ntohd() (the new version) which was written with htonl() (the old version), you get an error. But this shouldn't be a problem, because htonl() was in your copy of the brlcad repository only. It didn't made it into the official version.

Looks like this may be the problem. I will soon test it out and will report.

@H1manshu21
Copy link
Contributor Author

H1manshu21 commented Jan 26, 2022

Looks like everything is fine now. I have tested it by creating and also by opening it. @drossberg
Screenshot (27)
annot18.zip

@H1manshu21
Copy link
Contributor Author

One thing I noticed in line no 1209 of annot.c scan = cseg->radius * local2mm; that is multiplied fastf_t by local2mm. Is it converted fastf_t to double and then store in scan which is a type of double?

@H1manshu21
Copy link
Contributor Author

@brlcad I have a doubt. After I do git pull --rebase and do git push, I am not able to push since they are got rejected. And if I do pull and push from main to test without rebase then all the main commits got pushed to test and this PR always fills ith other commits which are not mine and make ugly to this PR. What I am doing wrong?

@H1manshu21
Copy link
Contributor Author

Whenever I sync my main branch with the upstream branch and wanted also that my test branch also should be in sync with the main branch, I do rebase but git push is unable to push those commits. So I have to do a force push in order to make sync.

@brlcad
Copy link
Member

brlcad commented Jan 27, 2022

@brlcad I have a doubt. After I do git pull --rebase and do git push, I am not able to push since they are got rejected. And if I do pull and push from main to test without rebase then all the main commits got pushed to test and this PR always fills ith other commits which are not mine and make ugly to this PR. What I am doing wrong?

I'm not 100% sure, could be several things. Are you updating your local main branch (i.e., Himanshu40:main) from BRL-CAD:main? If not, and only merging BRL-CAD:main into your PR feature branch, then that would explain why those commits would show up here. You'd be introducing commits to Himanshu40:test that do not exist on Himanshu40:main, so github shows them all as incoming commits. I think if you git pull on Himanshu40:main, then you should be able to rebase and push your test branch without issue, without forcing.

@brlcad
Copy link
Member

brlcad commented Jan 27, 2022

Whenever I sync my main branch with the upstream branch and wanted also that my test branch also should be in sync with the main branch, I do rebase but git push is unable to push those commits. So I have to do a force push in order to make sync.

What's the message you get when you try to push those commits? Not a great workaround, but we have a rebase and merge option here in the PR, so you don't need to rebase. You just need to make sure the PR has the commits that you want included. If the PR says "Merging can be performed automatically", then you have nothing to worry about. If it cannot, then you should totally do a merge commit to fix whatever conflict is preventing it from merging successfully. That's a case when merge commits are okay.

@brlcad
Copy link
Member

brlcad commented Jan 27, 2022

Looks like everything is fine now. I have tested it by creating and also by opening it. [snip])

Curious, is the bottom-left annotation positioned differently from all the rest? All the other annotations appear to be right-bottom aligned, yet left-top was specified. Hard to tell from that view without a rotation, but odd that one is inconsistent..

@H1manshu21
Copy link
Contributor Author

H1manshu21 commented Jan 27, 2022

Looks like everything is fine now. I have tested it by creating and also by opening it. [snip])

Curious, is the bottom-left annotation positioned differently from all the rest? All the other annotations appear to be right-bottom aligned, yet left-top was specified. Hard to tell from that view without a rotation, but odd that one is inconsistent..

My bad. I have specified the right top for vertices 1 and all the rest vertices to the left top.
Screenshot (28)

@H1manshu21
Copy link
Contributor Author

@brlcad I have a doubt. After I do git pull --rebase and do git push, I am not able to push since they are got rejected. And if I do pull and push from main to test without rebase then all the main commits got pushed to test and this PR always fills ith other commits which are not mine and make ugly to this PR. What I am doing wrong?

I'm not 100% sure, could be several things. Are you updating your local main branch (i.e., Himanshu40:main) from BRL-CAD:main? If not, and only merging BRL-CAD:main into your PR feature branch, then that would explain why those commits would show up here. You'd be introducing commits to Himanshu40:test that do not exist on Himanshu40:main, so github shows them all as incoming commits. I think if you git pull on Himanshu40:main, then you should be able to rebase and push your test branch without issue, without forcing.

Yes, I am updating my local main branch to BRL-CAD:main and push my local main to remote Himanshu:BRLCAD:main to sync. Then I switch to test branch to also sync my test branch with my updated local branch and I did git pull --rebase origin main and then I git push where it fails.

@H1manshu21
Copy link
Contributor Author

Whenever I sync my main branch with the upstream branch and wanted also that my test branch also should be in sync with the main branch, I do rebase but git push is unable to push those commits. So I have to do a force push in order to make sync.

What's the message you get when you try to push those commits? Not a great workaround, but we have a rebase and merge option here in the PR, so you don't need to rebase. You just need to make sure the PR has the commits that you want included. If the PR says "Merging can be performed automatically", then you have nothing to worry about. If it cannot, then you should totally do a merge commit to fix whatever conflict is preventing it from merging successfully. That's a case when merge commits are okay.

hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Integrate the remote changes (e.g.
hint: 'git pull ...') before pushing again.

@H1manshu21
Copy link
Contributor Author

When my main branch is out of sync with remote brlcad:main, I do this and also to sync my test branch with brlcad:main I perform this but it gets rejected and I have to force push to sync :-

$ git checkout main
$ git pull --rebase upstream main
$ git push origin main
$ git checkout test
$ git pull --rebase origin main
$ git push origin test

@dashohoxha
Copy link

You don't need to do all of these. You should work the normal way:

$ git checkout test

... edit files ...
$ git commit ...
$ git push

... edit files ...
$ git commit ...
$ git push

Finally, when the PR is about to be merged, you select the option "Rebase and merge" instead of "Create a merge commit".

@drossberg drossberg merged commit 98d01c7 into BRL-CAD:main Jan 31, 2022
@H1manshu21 H1manshu21 deleted the test branch January 31, 2022 19:15
zhuodannychen pushed a commit to zhuodannychen/brlcad that referenced this pull request May 9, 2023
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.

4 participants