Partially fix Ultima Underworld mouse issuesfc/ultima-underworld-mouse-fix-1

3 files changed, 205 insertions, 91 deletions

diff --git a/src/ints/mouse.cpp b/src/ints/mouse.cpp
index 23a762b72..6b299c53f 100644
--- a/src/ints/mouse.cpp
+++ b/src/ints/mouse.cpp
@@ -49,9 +49,20 @@ static bool raw_input = true; // true = relative input is raw data, without
 static Bitu int74_ret_callback = 0;
 
 // Original DOSBox forces mouse event delay of 5 milliseconds all the time, but
-// this is probably slightly too much - 7 milliseconds is already about twice the
-// typical 70 Hz screen refresh rate.
-static constexpr uint8_t boost_delay_ms = 7;
+// this is probably slightly too much - 8 milliseconds is already over twice the
+// 60 Hz rate the original Microsoft Mouse 8.20 driver sets
+// At least Ultima Underworld I and II do not like too high sampling rates
+static constexpr uint8_t boost_delay_ms = 8;
+
+static constexpr uint8_t mask_mouse_has_moved = static_cast<uint8_t>(MouseEventId::MouseHasMoved);
+static constexpr uint8_t mask_wheel_has_moved = static_cast<uint8_t>(MouseEventId::WheelHasMoved);
+static constexpr uint8_t mask_pressed  = static_cast<uint8_t>(MouseEventId::PressedLeft) +
+								         static_cast<uint8_t>(MouseEventId::PressedRight) +
+								         static_cast<uint8_t>(MouseEventId::PressedMiddle);
+static constexpr uint8_t mask_released = static_cast<uint8_t>(MouseEventId::ReleasedLeft) +
+								         static_cast<uint8_t>(MouseEventId::ReleasedRight) +
+								         static_cast<uint8_t>(MouseEventId::ReleasedMiddle);
+static constexpr uint8_t mask_button = mask_pressed + mask_released;
 
 // ***************************************************************************
 // Debug code, normally not enabled
@@ -172,20 +183,8 @@ private:
     bool event_dos_moved = false;
     bool event_dos_wheel = false;
 
-    bool prefer_ps2_next_time = false;
-
     uint32_t pic_ticks_start = 0; // PIC_Ticks value when timer starts
 
-    // Helper masks for aggregating DOS button events
-    static constexpr uint8_t aggregate_mask_pressed =
-            static_cast<uint8_t>(MouseEventId::PressedLeft)  |
-            static_cast<uint8_t>(MouseEventId::PressedRight) |
-            static_cast<uint8_t>(MouseEventId::PressedMiddle);
-    static constexpr uint8_t aggregate_mask_released =
-            static_cast<uint8_t>(MouseEventId::ReleasedLeft)  |
-            static_cast<uint8_t>(MouseEventId::ReleasedRight) |
-            static_cast<uint8_t>(MouseEventId::ReleasedMiddle);
-
     // Helpers to check if there are events in the queue
     bool HasEventDosMoved() const;
     bool HasEventDosButton() const;
@@ -322,11 +321,11 @@ void MouseQueue::AggregateEventsDOS(MouseEvent &event)
     // Generate masks to detect whether two button events can be
     // aggregated (might be needed later even if we have no more
     // events now)
-    if (event.mask & aggregate_mask_pressed)
+    if (event.mask & mask_pressed)
         // Set 'pressed+released' for every 'pressed' bit
         event.aggregate_mask = static_cast<uint8_t>(event.mask |
                                                     (event.mask << 1));
-    else if (event.mask & aggregate_mask_released)
+    else if (event.mask & mask_released)
         // Set 'pressed+released' for every 'released' bit
         event.aggregate_mask = static_cast<uint8_t>(event.mask |
                                                     (event.mask >> 1));
@@ -392,14 +391,8 @@ void MouseQueue::FetchEvent(MouseEvent &event)
         return;
     }
 
-    // We should prefer PS/2 events now (as the last was DOS one),
-    // but we can't if there is no PS/2 event ready to be handled
-    if (!HasReadyEventPS2())
-        prefer_ps2_next_time = false;
-
     // Try DOS button events
-    if (HasReadyEventDosButton() && !prefer_ps2_next_time) {
-        prefer_ps2_next_time = true;
+    if (HasReadyEventDosButton()) {
         // Set delay before next DOS events
         delay.dos_button_ms = start_delay.dos_button_ms;
         delay.dos_moved_wheel_ms = std::max(delay.dos_moved_wheel_ms,
@@ -412,7 +405,6 @@ void MouseQueue::FetchEvent(MouseEvent &event)
 
     // Now try PS/2 event
     if (HasReadyEventPS2()) {
-        prefer_ps2_next_time = false;
         // Set delay before next PS/2 events
         delay.ps2_ms = start_delay.ps2_ms;
         // PS/2 events are really dummy - merely a notification
@@ -640,8 +632,18 @@ static Bitu int74_handler()
                 event.request_ps2 ? "PS2" : "---",
                 event.request_dos ? "DOS" : "---");
 
-    // If DOS driver is active, use it to handle the event
-    if (event.request_dos && mouse_shared.active_dos) {
+    // Handle DOS events
+    if (event.request_dos) {
+        if ((event.mask & mask_mouse_has_moved) &&
+            !MOUSEDOS_UpdateMoved())
+            event.mask &= static_cast<uint8_t>(~mask_mouse_has_moved & 0xff);
+        if ((event.mask & mask_wheel_has_moved) &&
+            !MOUSEDOS_UpdateWheel())
+            event.mask &= static_cast<uint8_t>(~mask_wheel_has_moved & 0xff);
+        if ((event.mask & mask_button) &&
+            !MOUSEDOS_UpdateButtons(event.buttons_12S))
+            event.mask &= static_cast<uint8_t>(~mask_button & 0xff);
+
         // Taken from DOSBox X: HERE within the IRQ 12 handler is the
         // appropriate place to redraw the cursor. OSes like Windows 3.1
         // expect real-mode code to do it in response to IRQ 12, not
@@ -743,27 +745,36 @@ void MOUSE_NewScreenParams(const uint16_t clip_x, const uint16_t clip_y,
     MOUSE_NotifyStateChanged();
 }
 
+uint8_t clamp_start_delay(float value_ms)
+{
+    constexpr long min_ms = 3;   // 330 Hz sampling rate
+    constexpr long max_ms = 100; //  10 Hz sampling rate
+
+    const auto tmp = std::clamp(std::lround(value_ms), min_ms, max_ms);
+    return static_cast<uint8_t>(tmp);
+}
+
 void MOUSE_NotifyRateDOS(const uint8_t rate_hz)
 {
     auto &val_moved_wheel = queue.start_delay.dos_moved_wheel_ms;
     auto &val_button = queue.start_delay.dos_button_ms;
 
     // Convert rate in Hz to delay in milliseconds
-    val_moved_wheel = static_cast<uint8_t>(1000 / rate_hz);
+    val_moved_wheel = clamp_start_delay(1000.0f / rate_hz);
+
     // Cheat a little, do not allow to set too high delay. Some old
     // games might have tried to set lower rate to reduce number of IRQs
     // and save CPU power - this is no longer necessary. Windows 3.1 also
     // sets a suboptimal value in its PS/2 driver.
     val_moved_wheel = std::min(val_moved_wheel, boost_delay_ms);
+
     // Cheat a little once again - our delay for buttons is separate and
     // much smaller, so that button events can be sent to the DOS
-    // application with minimal latency. Due to hardware differences and
-    // multiple independent mouse driver implementations in the past, it is
-    // unlikely to cause problems. Besides, host OS and hardware have their
-    // limitations, too, they also introduce some latency.
-    val_button = val_moved_wheel / 5;
+    // application with minimal latency. So far this didin't cause any issues.
+    val_button = clamp_start_delay(val_moved_wheel / 5.0f);
 
-    // TODO: make a configuration option(s) for this, same for PS/2 mouse
+    // TODO: make a configuration option(s) for boosting sampling rates,
+	//       for DOS and PS/2 mice alike
 }
 
 void MOUSE_NotifyRatePS2(const uint8_t rate_hz)
@@ -771,7 +782,8 @@ void MOUSE_NotifyRatePS2(const uint8_t rate_hz)
     auto &val_ps2 = queue.start_delay.ps2_ms;
 
     // Convert rate in Hz to delay in milliseconds
-    val_ps2 = static_cast<uint8_t>(1000 / rate_hz);
+    val_ps2 = clamp_start_delay(1000.0f / rate_hz);
+
     // Cheat a little, like with DOS driver
     val_ps2 = std::min(val_ps2, boost_delay_ms);
 }
@@ -894,7 +906,7 @@ void MOUSE_EventPressed(uint8_t idx)
     }
     if (changed_12S) {
         event.request_vmm = MOUSEVMM_NotifyPressedReleased(buttons_12S);
-        event.request_dos = MOUSEDOS_NotifyPressed(buttons_12S, idx_12S);
+        event.request_dos = true;
     }
 
     queue.AddEvent(event);
@@ -948,7 +960,7 @@ void MOUSE_EventReleased(uint8_t idx)
                                                        get_buttons_joined());
     if (changed_12S) {
         event.request_vmm = MOUSEVMM_NotifyPressedReleased(buttons_12S);
-        event.request_dos = MOUSEDOS_NotifyReleased(buttons_12S, idx_12S);
+        event.request_dos = true;
         MOUSESERIAL_NotifyPressedReleased(buttons_12S, idx_12S);
     }
 
diff --git a/src/ints/mouse_core.h b/src/ints/mouse_core.h
index 08afa0433..3ce1a5c2c 100644
--- a/src/ints/mouse_core.h
+++ b/src/ints/mouse_core.h
@@ -248,10 +248,14 @@ uintptr_t MOUSEDOS_DoCallback(const uint8_t mask, const MouseButtons12S buttons_
 // - understands up to 3 buttons
 // - needs index of button which changed state
 
-bool MOUSEDOS_NotifyMoved(const float x_rel, const float y_rel,
-                          const uint16_t x_abs, const uint16_t y_abs);
-bool MOUSEDOS_NotifyPressed(const MouseButtons12S buttons_12S, const uint8_t idx);
-bool MOUSEDOS_NotifyReleased(const MouseButtons12S buttons_12S, const uint8_t idx);
+bool MOUSEDOS_NotifyMoved(const float x_rel,
+                          const float y_rel,
+                          const uint16_t x_abs,
+                          const uint16_t y_abs);
 bool MOUSEDOS_NotifyWheel(const int16_t w_rel);
 
+bool MOUSEDOS_UpdateMoved();
+bool MOUSEDOS_UpdateButtons(const MouseButtons12S buttons_12S);
+bool MOUSEDOS_UpdateWheel();
+
 #endif // DOSBOX_MOUSE_CORE_H
diff --git a/src/ints/mouse_dos_driver.cpp b/src/ints/mouse_dos_driver.cpp
index bc144cd66..5b95c6897 100644
--- a/src/ints/mouse_dos_driver.cpp
+++ b/src/ints/mouse_dos_driver.cpp
@@ -58,9 +58,29 @@ enum class MouseCursor : uint8_t { Software = 0, Hardware = 1, Text = 2 };
 static MouseButtons12S buttons = 0;
 static float pos_x = 0.0f;
 static float pos_y = 0.0f;
-static int16_t wheel_counter = 0;
+static int8_t counter_w = 0;
 static uint8_t rate_hz = 0; // TODO: add proper reaction for 0 (disable driver)
 
+static struct {
+    // Data from mouse events which were already received,
+    // but not necessary visible to the application
+
+    // Mouse movement
+    float x_rel    = 0.0f;
+    float y_rel    = 0.0f;
+    uint16_t x_abs = 0;
+    uint16_t y_abs = 0;
+
+    // Wheel movement
+    int16_t w_rel  = 0;
+
+    void Reset() {
+        x_rel = 0.0f;
+        y_rel = 0.0f;
+        w_rel = 0;
+    }
+} pending;
+
 static struct { // DOS driver state
 
     // Structure containing (only!) data which should be
@@ -157,6 +177,15 @@ static RealPt user_callback;
 // Common helper routines
 // ***************************************************************************
 
+static uint8_t signed_to_reg8(const int8_t x)
+{
+    if (x >= 0)
+        return static_cast<uint8_t>(x);
+    else
+        // -1 for 0xff, -2 for 0xfe, etc.
+        return static_cast<uint8_t>(0x100 + x);
+}
+
 static uint16_t signed_to_reg16(const int16_t x)
 {
     if (x >= 0)
@@ -180,6 +209,15 @@ static int16_t reg_to_signed16(const uint16_t x)
         return static_cast<int16_t>(x);
 }
 
+int8_t clamp_to_int8(const int32_t val)
+{
+    const auto tmp = std::clamp(val,
+                                static_cast<int32_t>(INT8_MIN),
+                                static_cast<int32_t>(INT8_MAX));
+
+    return static_cast<int8_t>(tmp);
+}
+
 static uint16_t get_pos_x()
 {
     return static_cast<uint16_t>(std::lround(pos_x)) & state.granularity_x;
@@ -526,14 +564,11 @@ static uint8_t get_reset_wheel_8bit()
     if (!state.cute_mouse) // wheel requires CuteMouse extensions
         return 0;
 
-    const int16_t tmp = static_cast<int16_t>(
-            std::clamp(wheel_counter,
-                       static_cast<int16_t>(INT8_MIN),
-                       static_cast<int16_t>(INT8_MAX)));
-    wheel_counter = 0; // reading always clears the counter
+    const auto tmp = counter_w;
+    counter_w = 0; // reading always clears the counter
 
     // 0xff for -1, 0xfe for -2, etc.
-    return static_cast<uint8_t>((tmp >= 0) ? tmp : (0x100 + tmp));
+    return signed_to_reg8(tmp);
 }
 
 static uint16_t get_reset_wheel_16bit()
@@ -541,11 +576,10 @@ static uint16_t get_reset_wheel_16bit()
     if (!state.cute_mouse) // wheel requires CuteMouse extensions
         return 0;
 
-    const int16_t tmp = wheel_counter;
-    wheel_counter = 0; // reading always clears the counter
+    const int16_t tmp = counter_w;
+    counter_w = 0; // reading always clears the counter
 
-    // 0xffff for -1, 0xfffe for -2, etc.
-    return static_cast<uint16_t>((tmp >= 0) ? tmp : (0x10000 + tmp));
+    return signed_to_reg16(tmp);
 }
 
 static void set_mickey_pixel_rate(const int16_t ratio_x, const int16_t ratio_y)
@@ -611,9 +645,10 @@ static void set_interrupt_rate(const uint16_t rate_id)
 
 static void reset_hardware()
 {
-    wheel_counter = 0;
+    counter_w = 0;
     set_interrupt_rate(4);
     PIC_SetIRQMask(12, false); // lower IRQ line
+    MOUSE_NotifyRateDOS(60); // same rate Microsoft Mouse 8.20 sets
 }
 
 void MOUSEDOS_BeforeNewVideoMode()
@@ -695,6 +730,11 @@ void MOUSEDOS_AfterNewVideoMode(const bool setmode)
 
 static void reset()
 {
+    // Although these do not belong to the driver state,
+    // reset them too to avoid any possible problems
+    counter_w = 0;
+    pending.Reset();
+	
     MOUSEDOS_BeforeNewVideoMode();
     MOUSEDOS_AfterNewVideoMode(false);
 
@@ -835,8 +875,7 @@ static void move_cursor_seamless(const float x_rel, const float y_rel,
     }
 }
 
-bool MOUSEDOS_NotifyMoved(const float x_rel, const float y_rel,
-                          const uint16_t x_abs, const uint16_t y_abs)
+bool MOUSEDOS_UpdateMoved()
 {
     const auto old_pos_x = get_pos_x();
     const auto old_pos_y = get_pos_y();
@@ -844,13 +883,17 @@ bool MOUSEDOS_NotifyMoved(const float x_rel, const float y_rel,
     const auto old_mickey_x = static_cast<int16_t>(state.mickey_x);
     const auto old_mickey_y = static_cast<int16_t>(state.mickey_y);
 
-    const auto x_mov = MOUSE_ClampRelativeMovement(x_rel * sensitivity_dos);
-    const auto y_mov = MOUSE_ClampRelativeMovement(y_rel * sensitivity_dos);
+    const auto x_mov = MOUSE_ClampRelativeMovement(pending.x_rel * sensitivity_dos);
+    const auto y_mov = MOUSE_ClampRelativeMovement(pending.y_rel * sensitivity_dos);
+
+    // Pending relative movement is now consummed
+    pending.x_rel = 0.0f;
+    pending.y_rel = 0.0f;
 
     if (mouse_is_captured)
         move_cursor_captured(x_mov, y_mov);
     else
-        move_cursor_seamless(x_mov, y_mov, x_abs, y_abs);
+        move_cursor_seamless(x_mov, y_mov, pending.x_abs, pending.y_abs);
 
     // Make sure cursor stays in the range defined by application
     limit_coordinates();
@@ -863,8 +906,6 @@ bool MOUSEDOS_NotifyMoved(const float x_rel, const float y_rel,
                               static_cast<int16_t>(state.mickey_x)) ||
                              (old_mickey_y !=
                               static_cast<int16_t>(state.mickey_y));
-    if (!abs_changed && !rel_changed)
-        return false;
 
     // NOTE: It might be tempting to optimize the flow here, by skipping
     // the whole event-queue-callback flow if there is no callback
@@ -874,53 +915,110 @@ bool MOUSEDOS_NotifyMoved(const float x_rel, const float y_rel,
     // constantly (don't ask me, why) - doing too much optimization
     // can cause the game to skip mouse events.
 
-    return true;
+    return abs_changed || rel_changed;
 }
 
-bool MOUSEDOS_NotifyPressed(const MouseButtons12S new_buttons_12S, const uint8_t idx)
+bool MOUSEDOS_UpdateButtons(const MouseButtons12S new_buttons_12S)
 {
-    assert(idx < num_buttons);
+    if (buttons.data == new_buttons_12S.data)
+        return false;
 
-    buttons = new_buttons_12S;
+    auto mark_pressed = [](const uint8_t idx) {
+        state.last_pressed_x[idx] = get_pos_x();
+        state.last_pressed_y[idx] = get_pos_y();
+    };
+
+    auto mark_released = [](const uint8_t idx) {
+        state.last_released_x[idx] = get_pos_x();
+        state.last_released_y[idx] = get_pos_y();
+    };
+
+    if (new_buttons_12S.left && !buttons.left)
+        mark_pressed(0);
+    else if (!new_buttons_12S.left && buttons.left)
+        mark_released(0);
+
+    if (new_buttons_12S.right && !buttons.right)
+        mark_pressed(1);
+    else if (!new_buttons_12S.right && buttons.right)
+        mark_released(1);
 
-    ++state.times_pressed[idx];
-    state.last_pressed_x[idx] = get_pos_x();
-    state.last_pressed_y[idx] = get_pos_y();
+    if (new_buttons_12S.middle && !buttons.middle)
+        mark_pressed(2);
+    else if (!new_buttons_12S.middle && buttons.middle)
+        mark_released(2);
 
-    // See comment in 'MOUSEDOS_NotifyMoved' about optimizing this
+    buttons = new_buttons_12S;
     return true;
 }
 
-bool MOUSEDOS_NotifyReleased(const MouseButtons12S new_buttons_12S, const uint8_t idx)
+bool MOUSEDOS_UpdateWheel()
 {
-    assert(idx < num_buttons);
+    counter_w = clamp_to_int8(static_cast<int32_t>(counter_w + pending.w_rel));
 
-    buttons = new_buttons_12S;
+    // Pending wheel scroll is now consummed
+    pending.w_rel = 0;
 
-    ++state.times_released[idx];
-    state.last_released_x[idx] = get_pos_x();
-    state.last_released_y[idx] = get_pos_y();
+    state.last_wheel_moved_x = get_pos_x();
+    state.last_wheel_moved_y = get_pos_y();
 
-    // See comment in 'MOUSEDOS_NotifyMoved' about optimizing this
-    return true;
+    return (counter_w != 0);
+}
+
+bool MOUSEDOS_NotifyMoved(const float x_rel,
+                          const float y_rel,
+                          const uint16_t x_abs,
+                          const uint16_t y_abs)
+{
+    // Check if an event is needed
+    bool event_needed = false;
+    if (mouse_is_captured) {
+        // Uses relative mouse movements - processing is too complicated
+        // to easily predict whether the event can be safely omitted
+        event_needed = true;
+        // TODO: it actually can be done - but it will require some refactoring
+    } else {
+        // Uses absolute mouse position (seamless mode), relative movements
+        // can wait to be reported - they are completely unreliable anyway
+        if (pending.x_abs != x_abs || pending.y_abs != y_abs)
+            event_needed = true;
+    }
+
+    // Update values to be consummed when the event arrives
+    pending.x_rel = MOUSE_ClampRelativeMovement(pending.x_rel + x_rel);
+    pending.y_rel = MOUSE_ClampRelativeMovement(pending.y_rel + y_rel);
+    pending.x_abs = x_abs;
+    pending.y_abs = y_abs;
+
+    // NOTES:
+	//
+	// It might be tempting to optimize the flow here, by skipping
+    // the whole event-queue-callback flow if there is no callback
+    // registered, no graphic cursor to draw, etc. Don't do this - there
+    // is at least one game (Master of Orion II), which performs INT 0x33
+    // calls with 0x0f parameter (changing the callback settings)
+    // constantly (don't ask me, why) - doing too much optimization
+    // can cause the game to skip mouse events.
+    //
+	// Also, do not update mouse state here - Ultima Underworld I and II
+	// do not like mouse states updated in real time, it ends up with
+	// mouse movements being ignored by the game randomly.
+
+    return event_needed;
 }
 
 bool MOUSEDOS_NotifyWheel(const int16_t w_rel)
 {
-    if (!state.cute_mouse) // wheel only available if CuteMouse extensions
-                           // are active
+    if (!state.cute_mouse)
         return false;
 
-    const auto tmp = std::clamp(static_cast<int32_t>(w_rel + wheel_counter),
-                                static_cast<int32_t>(INT16_MIN),
-                                static_cast<int32_t>(INT16_MAX));
+    // Although in some places it is possible for the guest code to get
+    // wheel counter in 16-bit format, scrolling hundreds of lines in one
+    // go would be insane - thus, limit the wheel counter to 8 bits and
+    // reuse the code written for other mouse modules
+    pending.w_rel = clamp_to_int8(pending.w_rel + w_rel);
 
-    wheel_counter            = static_cast<int16_t>(tmp);
-    state.last_wheel_moved_x = get_pos_x();
-    state.last_wheel_moved_y = get_pos_y();
-
-    // See comment in 'MOUSEDOS_NotifyMoved' about optimizing this
-    return true;
+    return (pending.w_rel != 0);
 }
 
 static Bitu int33_handler()
@@ -1117,11 +1215,11 @@ static Bitu int33_handler()
         MOUSEDOS_DrawCursor();
         break;
     case 0x11: // CuteMouse - get mouse capabilities
-        reg_ax           = 0x574d; // Identifier for detection purposes
-        reg_bx           = 0;      // Reserved capabilities flags
-        reg_cx           = 1;      // Wheel is supported
+        reg_ax    = 0x574d; // Identifier for detection purposes
+        reg_bx    = 0;      // Reserved capabilities flags
+        reg_cx    = 1;      // Wheel is supported
+        counter_w = 0;
         state.cute_mouse = true;   // This call enables CuteMouse extensions
-        wheel_counter    = 0;
         // Previous implementation provided Genius Mouse 9.06 function
         // to get number of buttons
         // (https://sourceforge.net/p/dosbox/patches/32/), it was