stefanhendriks / Dune-II---The-Maker

A remake of the classic Dune 2 - The Building of a Dynasty (by Westwood Studios) with several enhancements. Like: higher screenresolutions, zooming, multiselect, skirmish play, etc.
https://www.dune2themaker.com
301 stars 26 forks source link

[PATCH] Fix sticky mouse state after special launched #590

Open cm8 opened 8 months ago

cm8 commented 8 months ago

https://github.com/stefanhendriks/Dune-II---The-Maker/blob/c307d16b7da71e435588d79b142052ef778f8c1f/controls/mousestates/cMouseDeployState.cpp#L93-L95

After a special launched (e.g. combo Deathhand / Harkonnen), the mouse may not return from MOUSESTATE_DEPLOY, but rather sticks to it. This is observable in the game by the crosshair cursor colored red.

Since the following logic snippet prevents identity of m_prevState and m_state it presumably should not occur, because m_prevState == MOUSESTATE_DEPLOY when onNotifyGameEvent handler is called, but rather because it is called twice or more times. Not having debugged this directly, this seems to be the only explanation for the game's observable, unexpected behavior. https://github.com/stefanhendriks/Dune-II---The-Maker/blob/c307d16b7da71e435588d79b142052ef778f8c1f/controls/cGameControlsContext.cpp#L217-L221

If onNotifyGameEvent is handled twice, or an even number of times, the current code toggles first to m_prevState, but consequently setting m_prevState == MOUSESTATE_DEPLOY doing this. On the second call it reverts/swaps these states again, such that the player is unable to further control game's actions (instead of waiting for another special being ready..).

There may be a better or more general solution to this, so instead of a PR, please review and test wip-d2tm-fix-sticky-deploy-mousestate.patch.zip

diff --git a/controls/cGameControlsContext.cpp b/controls/cGameControlsContext.cpp
index 92f5c301..0d91c95c 100644
--- a/controls/cGameControlsContext.cpp
+++ b/controls/cGameControlsContext.cpp
@@ -258,6 +258,10 @@ void cGameControlsContext::toPreviousState() {
     setMouseState(m_prevState);
 }

+bool cGameControlsContext::isPreviousState(eMouseState other) const {
+    return this->m_prevState == other;
+}
+
 bool cGameControlsContext::isState(eMouseState other) const {
     return this->m_state == other;
 }
diff --git a/controls/cGameControlsContext.h b/controls/cGameControlsContext.h
index 9f924a58..5a9ca81b 100644
--- a/controls/cGameControlsContext.h
+++ b/controls/cGameControlsContext.h
@@ -55,6 +55,7 @@ class cGameControlsContext : public cInputObserver, cScenarioObserver {

         void toPreviousState();

+        bool isPreviousState(eMouseState other) const;
         bool isState(eMouseState other) const;

         void onFocusMouseStateEvent();
diff --git a/controls/mousestates/cMouseDeployState.cpp b/controls/mousestates/cMouseDeployState.cpp
index 0ef6da09..edaf558a 100644
--- a/controls/mousestates/cMouseDeployState.cpp
+++ b/controls/mousestates/cMouseDeployState.cpp
@@ -91,7 +91,11 @@ void cMouseDeployState::onFocus() {
 }

 void cMouseDeployState::onNotifyGameEvent(const s_GameEvent &) {
-    m_context->toPreviousState();
+    if (m_context->isPreviousState(MOUSESTATE_DEPLOY)) {
+        m_context->setMouseState(MOUSESTATE_SELECT);
+    } else {
+        m_context->toPreviousState();
+    }
 }

 void cMouseDeployState::onBlur() {

EDIT: a less invasive bugfix for the problem described is

diff --git a/controls/cGameControlsContext.cpp b/controls/cGameControlsContext.cpp
index 365fcfcf..49fdf4ea 100644
--- a/controls/cGameControlsContext.cpp
+++ b/controls/cGameControlsContext.cpp
@@ -229,8 +229,8 @@ void cGameControlsContext::onNotifyKeyboardEvent(const cKeyboardEvent &event) {

 void cGameControlsContext::setMouseState(eMouseState newState) {
     if (newState != m_state) {
-        // Remember previous state as long as it is not the PLACE state, since we can't go back to that state
-        if (m_state != eMouseState::MOUSESTATE_PLACE) {
+        // Remember previous state as long as it is not the PLACE or DEPLOY state
+        if (m_state != eMouseState::MOUSESTATE_PLACE && m_state != eMouseState::MOUSESTATE_DEPLOY) {
             m_prevState = m_state;
             if (newState == eMouseState::MOUSESTATE_REPAIR) {
                 m_prevStateBeforeRepair = m_state;
cm8 commented 8 months ago

If this bug exists to intentionally mimic behavior of the original 1990s game, and if people care to retain those bugs for nostalgic sake, eventually a command line option to d2tm executable makes sense as to activate 1990s conformance or not. But its more work to implement..