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

Merge endless-sky master to experimental, resolving Ship.cpp differences from refactor of Ship::Move #28

Merged

Conversation

UnorderedSigh
Copy link

@UnorderedSigh UnorderedSigh commented Jun 22, 2023

Feature: Updates experimental branch to master.

Feature Details

Git merge, did some conflict resolution, git add, etc.

Ship::Move Refactor

The hard part was Ship::Move in Ship.cpp which was refactored. I resolved those conflicts by manually comparing Ship.cpp between master and experimental before the refactor. Then I manually applied the changes in the right place.

Capture Uncapturable with Capturable Override test

This integration test kept failing for Ubuntu GL. It didn't like the test holding down "return" while waiting to board. I inserted a 30 frame wait loop before that, and the test works again.

TomGoodIdea and others added 30 commits May 10, 2023 15:54
* No longer show a message when the ship being scanned is the player's flagship or when the scan will result in a fine if successful.
…s-sky#8696)

This makes the settings more consistent and groups them together.
@UnorderedSigh
Copy link
Author

UnorderedSigh commented Jun 22, 2023

One of the integration tests kept failing for Ubuntu GL. It didn't like the test holding down "return" while waiting to board. I inserted a 30 frame wait loop before that, and the test works again.

Edit: It was the Capture Uncapturable with Capturable Override test.

@Zitchas
Copy link
Member

Zitchas commented Jun 22, 2023

Thanks for taking this on!

I've only had a chance to test the lateral thrust with and without automatic compensation on, and that looks like it's working. Haven't tested everything else yet, though.

@Zitchas
Copy link
Member

Zitchas commented Jun 26, 2023

Just as a potential reference thing:

Warp-core posted in #27 (comment) that they rebased Delta on master, and that this should be useful for us in updating is it helpful for you here?

@UnorderedSigh
Copy link
Author

Warp-core posted in #27 (comment) that they rebased Delta on master, and that this should be useful for us in updating is it helpful for you here?

It would not help when merging the refactor of Ship::Move. That must be done manually, regardless of your branch history.

@warp-core
Copy link

Warp-core posted in #27 (comment) that they rebased Delta on master, and that this should be useful for us in updating is it helpful for you here?

It would not help when merging the refactor of Ship::Move. That must be done manually, regardless of your branch history.

My branch has every commit unique to the experimental branch of Delta applied on top of the vanilla master branch (at the time). Resolving those conflicts was a necessary step of that.

@UnorderedSigh
Copy link
Author

My branch has every commit unique to the experimental branch of Delta applied on top of the vanilla master branch (at the time). Resolving those conflicts was a necessary step of that.

I did it in the opposite order: I merged master to experimental and resolved the conflicts.

@UnorderedSigh
Copy link
Author

UnorderedSigh commented Jun 28, 2023

I don't think merging a rebase via a pull request results in a rebase. To rebase the experimental branch, I'm pretty sure @Zitchas would have to force-push @warp-core's branch to experimental. Merging master seemed easier to me, so I did it that way.

Edit: By "easier," I mean you can merge it via a pull request.

@warp-core
Copy link

I don't think merging a rebase via a pull request results in a rebase. To rebase the experimental branch, I'm pretty sure @Zitchas would have to force-push @warp-core's branch to experimental. Merging master seemed easier to me, so I did it that way.

Edit: By "easier," I mean you can merge it via a pull request.

Indeed, merging a rebase of a branch into the original version via squash or merge commit doesn't seem like it'd go well, and seems like it'd defeat the point of rebasing.

If it were me, force pushing is how I would do it. Personally, I think rebasing should be the way the branch is kept up to date with vanilla master, rather than merge commits.

@UnorderedSigh
Copy link
Author

UnorderedSigh commented Jun 28, 2023

If it were me, force pushing is how I would do it.

That is risky and error-prone. It just takes one mistake, and you'll lose developments in experimental, and all history of them.

@UnorderedSigh
Copy link
Author

UnorderedSigh commented Jun 28, 2023

I tried merging master to this branch again, and I get warnings about merge conflicts. Most of the conflicts are genuine conflicts, such as changing version numbers. A few are git getting confused about past conflict resolution.

However, when I merge this branch to master, I get no conflicts. That means "experimental" can serve the critical purpose of a closed pull request to master, connectable to the ESLauncher2.

@UnorderedSigh
Copy link
Author

Manually examining changes in warp-core's branch, I see missing preferences:

diff --git a/source/PreferencesPanel.cpp b/source/PreferencesPanel.cpp
index 3abf143f6..35544a482 100644
--- a/source/PreferencesPanel.cpp
+++ b/source/PreferencesPanel.cpp
@@ -416,6 +416,7 @@ void PreferencesPanel::DrawControls()
                Command::SELECT,
                Command::SECONDARY,
                Command::CLOAK,
+               Command::MOUSE_TURNING_HOLD,
                Command::NONE,
                Command::MENU,
                Command::MAP,
@@ -551,25 +552,20 @@ void PreferencesPanel::DrawSettings()
                "Show asteroid scanner overlay",
                "Highlight player's flagship",
                "Rotate flagship in HUD",
-               FLAGSHIP_VELOCITY_INDICATOR,
-               "Show flagship data in HUD",
                "Show planet labels",
                "Show mini-map",
-               "Show asteroid scanner overlay",
-               "Always underline shortcuts",
                "Clickable radar display",
                ALERT_INDICATOR,
                "Extra fleet status messages",
                "\n",
                "Gameplay",
                "Control ship with mouse",
+               "Disable auto-stabilization",
                AUTO_AIM_SETTING,
                AUTO_FIRE_SETTING,
                TURRET_TRACKING,
                TARGET_ASTEROIDS_BASED_ON,
                BOARDING_PRIORITY,
-               "Control ship with mouse",
-               "Disable auto-stabilization",
                "Flagship flotsam collection",
                EXPEND_AMMO,
                FIGHTER_REPAIR,

@UnorderedSigh
Copy link
Author

UnorderedSigh commented Jun 28, 2023

@warp-core's branch is also missing my workaround that lets the tests pass after the merge. It turns out Ubuntu 20 & 22 GL don't like the player holding down the "return" button while waiting to board.

diff --git a/tests/integration/config/plugins/integration-tests/data/tests/tests_capture_override.txt b/tests/integration/config/plugins/integration-tests/data/tests/tests_capture_override.txt
index 7decb9fd8..39919ef84 100644
--- a/tests/integration/config/plugins/integration-tests/data/tests/tests_capture_override.txt
+++ b/tests/integration/config/plugins/integration-tests/data/tests/tests_capture_override.txt
@@ -108,6 +108,11 @@ test "Capture Uncapturable With Capturable Override"
                call "Depart"
                input
                        command board
+               label looping
+               apply
+                       "test: board loop" += 1
+               branch looping
+                       "test: board loop" < 30
                label "floating"
                # empty input to make time pass
                input

@warp-core
Copy link

@warp-core's branch is also missing my workaround that lets the tests pass after the merge. It turns out Ubuntu 22 doesn't like the player holding down the "return" button while waiting to board.

diff --git a/tests/integration/config/plugins/integration-tests/data/tests/tests_capture_override.txt b/tests/integration/config/plugins/integration-tests/data/tests/tests_capture_override.txt
index 7decb9fd8..39919ef84 100644
--- a/tests/integration/config/plugins/integration-tests/data/tests/tests_capture_override.txt
+++ b/tests/integration/config/plugins/integration-tests/data/tests/tests_capture_override.txt
@@ -108,6 +108,11 @@ test "Capture Uncapturable With Capturable Override"
                call "Depart"
                input
                        command board
+               label looping
+               apply
+                       "test: board loop" += 1
+               branch looping
+                       "test: board loop" < 30
                label "floating"
                # empty input to make time pass
                input

This doesn't appear to be in the experimental branch of delta, which is what I rebased.

It does appear some conflicts in the settings have been handled incorrectly.

If you don't think rebasing is a good way to handle bringing this up to date, don't do it.
If you think it is, feel free to correct the conflict resolution.

@UnorderedSigh
Copy link
Author

If you don't think rebasing is a good way to handle bringing this up to date, don't do it.
If you think it is, feel free to correct the conflict resolution.

I think @Zitchas needs to decide that.

@UnorderedSigh
Copy link
Author

I found the actual cause of the integration test failures (#29) and I removed my workaround that doesn't work. Once the master and experimental conflicts are resolved, I'll do a pull request. The diff is in that issue.

@Zitchas
Copy link
Member

Zitchas commented Jul 3, 2023

I think this looks good. Thank you warpcore for providing the alternate methodology. It is appreciated, particularly as a check.

@Zitchas Zitchas merged commit f80afe1 into endless-sky:experimental Jul 3, 2023
@Zitchas
Copy link
Member

Zitchas commented Jul 3, 2023

OK, I merged that as a squash-merge, but I think that undid a lot of the work of updating, so I'm thinking I will revert it and then do the commit-merge instead, and thus keeping the history and thus a more proper synchronization with mainstream master.

Zitchas added a commit that referenced this pull request Jul 3, 2023
…esolving Ship.cpp differences from refactor of Ship::Move (#28)"

This reverts commit f80afe1.
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.