OutpostUniverse / OPHD

OutpostHD - Open source remake of Sierra On-Line's Outpost
BSD 3-Clause "New" or "Revised" License
110 stars 21 forks source link

Fix PgUp/PgDown not changing structure palette #1445

Closed alynsarcana closed 7 months ago

alynsarcana commented 8 months ago

The number keys called a local function that handled changing the structure picker. This change adds a 2nd parameter (defaulting to null) to that local function, allowing the event handlers for PageUp/Down to call the same function, rather than call directly to the mMapView object. If the 2nd parameter have a value, then the 1st parameter is ignored and the 2nd is used to change the mMapView z level.


Edit: Closes #1364

DanRStevens commented 8 months ago

Oh interesting, and unexpected.

I edited the lead comment to link to the issue. Using the lead message with closes allows the issue to auto close when the PR is merged. I also adjusted the title. I figure a description is more useful in the title for browsing than an issue number. Plus, as I've just discovered, the issue doesn't get linked when referenced from the title, but does get linked when referenced in comments.

DanRStevens commented 8 months ago

Aha, I've discovered how to fetch the changes without having to first add a new remote for the fork. Seems there are some GitHub refs that can be used, which don't normally appear in branch lists.

For example:

git fetch origin pull/1445/head:fixPgUpPgDown
git co fixPgUpPgDown
git push -u origin fixPgUpPgDown

That's quite a bit more convenient than adding a new remote.

Anyway, tests are running. The Windows build can take somewhere from 5-15 minutes. I won't be sticking around though. Maybe the whitespace thing will be fixed before I revisit.

DanRStevens commented 8 months ago

When testing this locally I noticed one oddity, which I haven't really looked into yet. The hotkeys all work with these changes, but the mouse buttons no longer switch the construction menu. Checking against main, it seems the mouse buttons did switch the construction menu before.

It's curious because there were no obvious changes to the mouse button handling code. Maybe there were extra calls to handle the level change side effects which are no longer getting called due to the z-level change detection? I know a few places in the game checked certain conditions every frame drawn, not simply when a state change was made.

DanRStevens commented 8 months ago

In MapViewState::onMouseDown, there is a call to changeViewDepth, which should be updated to call onChangeDepth instead:

        const auto oldDepth = mMapView->currentDepth();
        mNavControl->onClick(MOUSE_COORDS);
        if (oldDepth != mMapView->currentDepth())
        {
            changeViewDepth(mMapView->currentDepth());
        }