Correct face orientation fixes #506 #628

gabsi26 · 2022-05-24T14:25:38Z

This PR simply checks if the path of a sweep is pointing in a negative direction.
The change is quite minimal and might not be sufficient for all cases

hannobraun

Thank you for the pull request, @gabsi26!

First off, I found that switching to an opaque color made it way easier to figure out what's going on. Maybe we should switch the model colors back to something simple, and just have one transparent model for testing, but that has nothing to do with this pull request.

This pull request addressed only part of #506, by fixing the top and bottom faces. It does not address the side faces. See here:

(That's just the star model, except h in line 59 was replaced with -h.)

I'd be happy to merge a partial fix, but the commit message of your first commit contains the magic incantation that will make GitHub close the issue upon merging this. Could you please either remove the reference to the issue from the commit message (straight-forward to do using something like git rebase -i main, in case you didn't know), or fix the side walls?

gabsi26 · 2022-05-24T15:20:02Z

Well i tried getting rid of the reference but it doesn't seem to work.
I'm not that fluent in guit unfortunatelly

hannobraun · 2022-05-24T15:24:06Z

Well i tried getting rid of the reference but it doesn't seem to work.
I'm not that fluent in guit unfortunatelly

I know that Git can be hard to deal with. Luckily (for me), it gets easier after 10+ years 😄

I'll take care of it. I'll post the steps I'm taking here, in case you're interested.

gabsi26 · 2022-05-24T15:27:49Z

Yes please that would be great as a learning resource :D

In the meantime I think i really fixed the issue^^

hannobraun · 2022-05-24T15:57:52Z

Oh man, that branch was truly broken 😄

Not sure what happened, but as a general note, git merge is often not a great option in pull requests, as it creates a complicated history. I suspect that in this case, there was some back-and-forth merging going on, seeing how confused the branch was.

In general, if your pull request is no longer up-to-date, the best way to address that is git rebase, which just takes your commits and replays them on top of another branch. If any merge conflict arises, you can fix them commit by commit. This leaves behind a linear history, without any merge commits with multiple parents.

Let's say, you're working on the branch my-fix, and have just pulled the latest changes from the upstream repository into main. Go to your branch (git switch my-fix) and run git rebase main, fix each conflict (if there are any). Once you fixed a conflict, use git rebase --continue to go to the next one. You can also use git rebase --skip to throw away one of your commits, or git rebase --abort, if you're in over your head and just want to go to where you were before you started rebasing.

Okay, but enough introduction. Here's what I did to fix the branch.

First, I tried a git rebase main to clean up the history. One complication was that Git tried to apply the same commit twice. Not sure why, but given how confusing the branch was, I'm not surprised Git does confusing things with it. When Git showed me the commit, I verified (using git log) that the commit it complained about was already applied, and used git rebase --skip to throw it away.

Next, I looked at the branch (git log) and saw it still contained lots of commits that didn't belong there. Again, not sure why. Fortunately, in this case it was pretty easy to distinguish between commits from your branch, and commits I've made myself earlier today. I decided to sort through them manually.

So, git rebase -i main (-i for interactive), and tell Git which ones I'd like to get rid of:

You can just remove the lines of the commits you want to drop instead of writing drop, but I figured this way it's easier to see what I did.

Saved the file, quit the editor, and Git went ahead.

git log now looks good.

I noticed at this point, that the reference to the issue was gone. Looks like your attempt to update the commit was successful in that regard. If it hadn't been gone, I'd have run another git rebase -i main, and marked the commit with r (for reword) instead of pick. Git should then take care of the rest, showing you an editor where you can edit the commit message.

Now I wanted to include your new commit, but didn't want to start over. So first, I told Git to get the latest version of your branch, without merging it into my local branch using git fetch.

You can see the reference to FETCH_HEAD here, which is a convenient shortcut for "whatever the thing is you just fetched". So now I can do a git log FETCH_HEAD.

Copy the commit id of your new commit, and get it into my local branch using git cherry-pick.

git cherry-pick is a bit like git merge, except it only merges exactly the commits you tell it to, not their history. So no risk of messing anything up again.

Now the branch looks good to me.

Next, force-push the repaired branch to GitHub (git push -f), and here it is. Refresh this page, and GitHub only shows those 3 commits.

Last step: Hope I didn't miss anything, because digging through forgotten history using git reflog is where the pain really starts 😄

Checks if the path points in negative z-direction and flips surface normal accordingly this is sufficient as long as sketches are always drawn in the xy-plane if this ever changes this has to be revisited Signed-off-by: gabsi26 <gabriel.seebach@gmx.at>

Signed-off-by: gabsi26 <gabriel.seebach@gmx.at>

hannobraun · 2022-05-24T16:04:10Z

While I'm already at it, some extra steps. The first commit makes a useful change with a bug. The second commit fixes that bug. If we combine them into one, the history becomes easier to read (and the pull request becomes easier to review).

So, another git rebase -i main, and tell Git we want to combine two commits.

Now Git tells me to combine the commit messages:

Which is not hard:

Success!

The simplified branch:

Another git push -f, and now your mistake is hidden forever, and no reviewer has to think about it 😁

Not essential, but I figured, while I was writing a little Git tutorial, why not include another useful technique.

gabsi26 · 2022-05-24T16:08:51Z

Thanks a lot for this indepth explanation and cleaning up my mess. I hope I won't cause something like this again, but at least nowthere is a guide how to fix it :D

hannobraun · 2022-05-24T16:12:14Z

Thanks a lot for this indepth explanation and cleaning up my mess. I hope I won't cause something like this again, but at least nowthere is a guide how to fix it :D

You're welcome! And don't worry, it happens. Git can be gnarly, but most of these problems are relatively easy to fix, once you understand what's happening and know enough Git commands to clean it up. Just takes time.

hannobraun

Thank again for this pull request, @gabsi26!

It looks good to me now. I've left some suggestions for improvements, but it's fine. Those things can be addressed at some later point. Feel free to submit a follow-up PR, if you're up for it, or just leave it, as you prefer.

Merging now.

hannobraun · 2022-05-24T16:07:10Z

crates/fj-kernel/src/algorithms/sweep.rs

@@ -124,11 +124,22 @@ pub fn sweep_shape(
 let top_edge =
 source_to_top.edges().get(&edge_source).unwrap().clone();

- let surface =
+ let surface = if path.dot(&Vector::from([0., 0., 1.]))


Would be nicer, if that check happened only once, and the result was stored into a variable with a descriptive name (sweep_along_negative_direction, or something like that).

With two separate checks, it becomes easy to miss one, once they need to change.

hannobraun · 2022-05-24T16:07:49Z

crates/fj-kernel/src/algorithms/sweep.rs

@@ -124,11 +124,22 @@ pub fn sweep_shape(
 let top_edge =
 source_to_top.edges().get(&edge_source).unwrap().clone();

- let surface =
+ let surface = if path.dot(&Vector::from([0., 0., 1.]))
+ >= fj_math::Scalar::from_f64(0.)


Note that Scalar has some short-hands for often-used values, in this case Scalar::ZERO.

hannobraun · 2022-05-24T16:10:20Z

crates/fj-kernel/src/algorithms/sweep.rs

+ target.insert(
+ Surface::SweptCurve(SweptCurve {
+ curve: bottom_edge.get().curve(),
+ path,
+ })
+ .reverse(), ////////////////////////////////////


Would be a bit nicer to reduce some duplication here. Something like this:

let mut surface = Surface::SweptCurve(...); if sweep_along_negative_direction { surface = surface.reverse(); } let surface = target.insert(surface)?;

gabsi26 requested a review from hannobraun as a code owner May 24, 2022 14:25

hannobraun requested changes May 24, 2022

View reviewed changes

hannobraun force-pushed the correct_face_orientation branch from 1be8e2d to a7bf716 Compare May 24, 2022 15:53

gabsi26 added 2 commits May 24, 2022 18:00

Sides of sweep are now also flipped if needed

b9384ab

Signed-off-by: gabsi26 <gabriel.seebach@gmx.at>

hannobraun force-pushed the correct_face_orientation branch from a7bf716 to b9384ab Compare May 24, 2022 16:03

hannobraun approved these changes May 24, 2022

View reviewed changes

hannobraun merged commit d2dd108 into hannobraun:main May 24, 2022

gabsi26 deleted the correct_face_orientation branch May 24, 2022 18:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correct face orientation fixes #506 #628

Correct face orientation fixes #506 #628

gabsi26 commented May 24, 2022

hannobraun left a comment

gabsi26 commented May 24, 2022

hannobraun commented May 24, 2022

gabsi26 commented May 24, 2022

hannobraun commented May 24, 2022

hannobraun commented May 24, 2022

gabsi26 commented May 24, 2022

hannobraun commented May 24, 2022

hannobraun left a comment

hannobraun May 24, 2022

hannobraun May 24, 2022

hannobraun May 24, 2022

Correct face orientation fixes #506 #628

Correct face orientation fixes #506 #628

Conversation

gabsi26 commented May 24, 2022

hannobraun left a comment

Choose a reason for hiding this comment

gabsi26 commented May 24, 2022

hannobraun commented May 24, 2022

gabsi26 commented May 24, 2022

hannobraun commented May 24, 2022

hannobraun commented May 24, 2022

gabsi26 commented May 24, 2022

hannobraun commented May 24, 2022

hannobraun left a comment

Choose a reason for hiding this comment

hannobraun May 24, 2022

Choose a reason for hiding this comment

hannobraun May 24, 2022

Choose a reason for hiding this comment

hannobraun May 24, 2022

Choose a reason for hiding this comment