Fix #40009: Win32 Use Message Time for Events not Current Time #115872

Merged
Harley Acheson merged 9 commits from Harley/blender:MessageTime into main 2023-12-08 18:59:41 +01:00
Member

When creating Blender events in Win32 message processing we are using
the current time as timestamp. This isn't set until we collect them, so
this might be inaccurate at times of high load. This PR changes to using
the time the message was delivered. Although GetMessageTime is a 32-bit
timer this combines it with a 64-bit time to avoid rollover.

When creating Blender events in Win32 message processing we are using the current time as timestamp. This isn't set until we collect them, so this might be inaccurate at times of high load. This PR changes to using the time the message was delivered. Although GetMessageTime is a 32-bit timer this combines it with a 64-bit time to avoid rollover.
Harley Acheson added 1 commit 2023-12-07 05:09:59 +01:00
When creating Blender events in Win32 message processing we are using
the current time as timestamp. This isn't set until we collect them, so
this might be inaccurate at times of high load. This PR changes to using
the time the message was delivered. Although GetMessageTime is a 32-bit
timer this combines it with a 64-bit time to avoid rollover.
Harley Acheson added this to the Platforms, Builds Tests & Devices project 2023-12-07 05:10:11 +01:00
Harley Acheson requested review from Ray molenkamp 2023-12-07 05:10:19 +01:00
Ray molenkamp reviewed 2023-12-07 06:40:31 +01:00
@ -206,0 +207,4 @@
{
int64_t t_now = getMilliSeconds();
/* Handles timer rollover. https://devblogs.microsoft.com/oldnewthing/20220107-00/?p=106130 */
int32_t diff = static_cast<uint32_t>(t_now) - GetMessageTime();
Member

I'm not saying it won't work, but i'm a bit uncomfortable combining data from 2 different time sources, getMilliSeconds() will get you the time since more or less process start in milliseconds and won't overflow (in our lifetime), GetMessageTime() will get the time of the event in milliseconds from system start and will rollover every 49ish days.

subtracting the two i suppose will get you a stable time stamp for events, the issue with it is its value absolutely meaningless. it's a stamp neither from OS boot nor from process start

My original solution here subtracted the 2 low precision timers (GetMessageTime - GetTickCount) to get a single offset and combined that with the getMilliSeconds() time to get a timestamp of the event, in milliseconds from process start.

I'm not saying it won't work, but i'm a bit uncomfortable combining data from 2 different time sources, `getMilliSeconds()` will get you the time since more or less process start in milliseconds and won't overflow (in our lifetime), `GetMessageTime()` will get the time of the event in milliseconds from system start and will rollover every 49ish days. subtracting the two i suppose will get you a stable time stamp for events, the issue with it is its value absolutely meaningless. it's a stamp neither from OS boot nor from process start My original solution here subtracted the 2 low precision timers (GetMessageTime - GetTickCount) to get a single offset and combined that with the `getMilliSeconds()` time to get a timestamp of the event, in milliseconds from process start.

To follow something like the conventions on X11/Wayland.

/* At beginning of event processing. */
uint32_t timestamp = GetMessageTime();
... snip ...
/* For event creation. */
const uint64_t event_ms = system->ms_from_input_time(timestamp);

... new GHOST_EventWheel(event_ms, window, -1);

Internally ms_from_input_time(..) can use getMilliSeconds, handle overflow etc to get a getMilliSeconds compatible time from the timestamp.


By convention I've also assigned a variable to the getMilliSeconds, it's good to note why the time-stamp can't be used since it's preferred where possible:

/* Comment explaining why the timestamp can't be used. */
const uint64_t event_ms = getMilliSeconds();
... new GHOST_Event(event_ms, GHOST_kEventWindowSize, window);
To follow something like the conventions on X11/Wayland. ``` /* At beginning of event processing. */ uint32_t timestamp = GetMessageTime(); ... snip ... /* For event creation. */ const uint64_t event_ms = system->ms_from_input_time(timestamp); ... new GHOST_EventWheel(event_ms, window, -1); ``` Internally `ms_from_input_time(..)` can use `getMilliSeconds`, handle overflow etc to get a `getMilliSeconds` compatible time from the timestamp. ---- By convention I've also assigned a variable to the `getMilliSeconds`, it's good to note why the time-stamp can't be used since it's preferred where possible: ``` /* Comment explaining why the timestamp can't be used. */ const uint64_t event_ms = getMilliSeconds(); ... new GHOST_Event(event_ms, GHOST_kEventWindowSize, window); ```
Harley Acheson added 1 commit 2023-12-07 18:44:38 +01:00
Ray molenkamp reviewed 2023-12-07 20:00:34 +01:00
@ -77,1 +77,4 @@
/**
* Returns the system time of the last message.
* Returns the number of milliseconds since the start of the system process
Member

This is false, it returns the number of milliseconds since the start of the blender process.

This is false, it returns the number of milliseconds since the start of the blender process.
Harley marked this conversation as resolved
Ray molenkamp approved these changes 2023-12-07 20:01:14 +01:00
Ray molenkamp left a comment
Member

lgtm, accepted contingent on fixing that one comment

lgtm, accepted contingent on fixing that one comment
Harley Acheson added 1 commit 2023-12-07 20:41:25 +01:00
Harley Acheson added 1 commit 2023-12-07 20:45:28 +01:00
formatting changes
Some checks failed
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
b1ec285003
Author
Member

@blender-bot build windows

@blender-bot build windows
Harley Acheson added 1 commit 2023-12-07 20:56:46 +01:00
Merge branch 'main' of projects.blender.org:blender/blender into MessageTime
Some checks failed
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
eebaac70f5
Author
Member

@blender-bot build windows

@blender-bot build windows
Campbell Barton reviewed 2023-12-07 21:07:33 +01:00
@ -205,1 +205,4 @@
uint64_t GHOST_SystemWin32::getMessageTime() const
{
/* Get difference between last message time and now. */

Worth mentioning the 49 day rollover isn't accounted for her.

Worth mentioning the 49 day rollover isn't accounted for her.
@ -78,0 +81,4 @@
* to the time of the message. This uses the high frequency timer if available.
* \return The number of milliseconds.
*/
uint64_t getMessageTime() const;

It would be good to mention when this should be used, that it should replace getMilliSeconds for events unless that event isn't associated with GetMessageTime (such as events Blender itself generates I assume).

It would be good to mention when this should be used, that it should replace `getMilliSeconds` for events unless that event isn't associated with `GetMessageTime` (such as events Blender itself generates I assume).
Member

Yeah it's probably good to document it should only ever be used for events generated from GHOST_SystemWin32::s_wndProc
we could protect it likely with some kind of assert inside getMessageTime to guarantee it, but that feels like bit overkill?

Yeah it's probably good to document it should only ever be used for events generated from `GHOST_SystemWin32::s_wndProc` we could protect it likely with some kind of assert inside `getMessageTime` to guarantee it, but that feels like bit overkill?

If this should only ever be used from s_wndProc why not make it a static function? Exposing as a method like this makes it seem as if it might be called anywhere.

If this should only ever be used from `s_wndProc` why not make it a static function? Exposing as a method like this makes it seem as if it might be called anywhere.
Harley Acheson added 1 commit 2023-12-07 22:05:13 +01:00
Author
Member

Now a static function. And handling rollover similar to 4a735b1d05 and comment changes.

Now a static function. And handling rollover similar to 4a735b1d05af64711873ea531cc6d30e4f33e016 and comment changes.
Harley Acheson added 1 commit 2023-12-07 22:09:05 +01:00
Comment correction.
Some checks failed
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
d74da0472c
Author
Member

@blender-bot build windows

@blender-bot build windows
Campbell Barton reviewed 2023-12-07 23:31:54 +01:00
@ -206,0 +215,4 @@
/* Handle 32-bit rollover. */
if (t_delta > 0) {
t_delta -= int64_t(UINT32_MAX);

In this case I think you want to wrap the whole range int64_t(UINT32_MAX) + 1.

In this case I think you want to wrap the whole range `int64_t(UINT32_MAX) + 1`.
Harley Acheson added 1 commit 2023-12-07 23:48:40 +01:00
rollover the entire range of int32
Some checks failed
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
6465f34df7
Author
Member

@blender-bot build windows

@blender-bot build windows
Harley Acheson added 1 commit 2023-12-08 18:31:30 +01:00
Merge branch 'main' of projects.blender.org:blender/blender into MessageTime
All checks were successful
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
dd2df5a712
Author
Member

@blender-bot build windows

@blender-bot build windows
Harley Acheson merged commit 0137e5494a into main 2023-12-08 18:59:41 +01:00
Harley Acheson deleted branch MessageTime 2023-12-08 18:59:43 +01:00
Sign in to join this conversation.
No reviewers
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#115872
No description provided.