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

Make CSGShape follow curve's tilt in Path mode #79355

Merged
merged 1 commit into from
Aug 17, 2023

Conversation

pidogs
Copy link
Contributor

@pidogs pidogs commented Jul 11, 2023

This is the simple fix for #65634.
The fix adds the tilt flag for sample_baked_up_vector in the csg_shape.cpp

Production edit: Fixes #65634

@pidogs pidogs requested a review from a team as a code owner July 11, 2023 23:39
@kleonc kleonc added this to the 4.2 milestone Jul 12, 2023
@kleonc kleonc added cherrypick:3.x Considered for cherry-picking into a future 3.x release cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release labels Jul 12, 2023
@YuriSizov YuriSizov changed the title Fixed path follow tilt in csg shape Make CSGShape follow curve's tilt in Path mode Jul 21, 2023
@YuriSizov
Copy link
Contributor

Could you please squash your commits into one? Make sure that the final commit has a short but descriptive message (the title of this PR is a good option). See this documentation, if you need help with squashing.

@pidogs
Copy link
Contributor Author

pidogs commented Jul 21, 2023

I think I put all the commits into one but now I'm not sure how to change 3bd5a98 into the new one.
I also created #79762 while trying to change it so if that could be deleted that would be great

@AThousandShips
Copy link
Member

PRs can't be deleted, only closed

@YuriSizov
Copy link
Contributor

YuriSizov commented Jul 24, 2023

I think I put all the commits into one

You haven't. You only merged master into your branch. So now it contains 3 commits that you've made and one that is a merge commit. First of all, you shouldn't merge master into your branches, you should rebase your branches on top of master. That means that your changes would always look like some version of master + your changes instead of some version of master + your changes + more changes from master.

You need to take your current branch and do the following:

  1. Fetch from all remotes to make sure your information about the state of the upstream master is correct.
  2. Take the topmost commit in the upstream master and initiate an interactive rebase. You can do it via any GUI application, if you use one, or via CLI with git rebase -i <commit_hash>.
  3. If you are doing it through CLI (and some GUI apps) you will be prompted to edit a file that looks like:
pick abcdef1234 Some commit message
pick 5678efabcd Some other commit message
pick 6543decfab Yet another commit message
pick 12de34ab45 Merge branch 'godotengine:master' into master

The file also contains brief instructions about how it can be edited.

  1. You want to keep your initial commit, fixup your additional commits, and drop the merge commit (now that you're properly rebasing).
  2. Keep the first line as is, in the second line erase pick and put f or fixup. In the final line, that should read "Merge branch 'godotengine:master' into master" replace pick with d or drop.
pick abcdef1234 Some commit message
f 5678efabcd Some other commit message
f 6543decfab Yet another commit message
d 12de34ab45 Merge branch 'godotengine:master' into master
  1. Save the file and close it, the rebase will continue and complete with all your commits squashed together.
  2. Now, edit the commit message with git commit --amend -m "Make CSGShape follow curve's tilt in Path mode".
  3. And now force-push your changes to the same branch as before.

If you're using a GUI app it may have dedicated UI for the rebase where you may pick what to keep and what to drop with some on-screen controls. Refer to your app's documentation for more details.

PS. I also recommend you avoid using your master branch when authoring commits. Create a feature branch from your master to make things easier for yourself. You don't have to do it now with this PR, but please consider it in future.

@akien-mga
Copy link
Member

I rebased and squashed the commits myself, and improved the commit message.

@akien-mga akien-mga merged commit 5aaaf76 into godotengine:master Aug 17, 2023
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@pidogs
Copy link
Contributor Author

pidogs commented Aug 17, 2023

Thank you for squashing the commits. I did not want to mess up the pull quests even more than I already did, I still have a lot to learn about git.
For future reference what is the best practice for forks and branches?

@akien-mga
Copy link
Member

We have a handy guide for this: https://docs.godotengine.org/en/latest/contributing/workflow/pr_workflow.html

But in general, you should keep your fork's master branch in sync with the upstream repo's master branch, and then create PR-specific branches from it. For example:

git remote add upstream https://github.com/godotengine/godot
git fetch upstream
git checkout master
git reset --hard upstream/master
# now your fork's master branch is _locally_ in sync with the upstream one
git push --force origin master
# this will also update your remote to be in sync with the upstream master branch

git checkout -b csgshape-path-follow-tilt
# new branch named "csgshape-path-follow-tilt" based on your up-to-date "master" branch
# you can do commits, push to your fork, and make a PR

@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Oct 19, 2023
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:3.x Considered for cherry-picking into a future 3.x release topic:core topic:3d
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CSGPolygon in Path mode does not take the curve's tilt property into account
6 participants