-
Notifications
You must be signed in to change notification settings - Fork 36
Cross-platform stability fixes and FP-contract standardization #283
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
base: development
Are you sure you want to change the base?
Changes from all commits
c8d7c0e
0ddf2dd
97526f9
1033146
0321d6a
19bf435
5272197
c579684
c141f41
7422555
f3ddc0a
630cf62
3e92187
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1891,7 +1891,7 @@ void Profiler::Worker() | |
|
|
||
| lastBroadcast = t; | ||
| const auto ts = std::chrono::duration_cast<std::chrono::seconds>( std::chrono::system_clock::now().time_since_epoch() ).count(); | ||
| broadcastMsg.activeTime = int32_t( ts - m_epoch ); | ||
| broadcastMsg.activeTime = ts > m_epoch ? int32_t( ts - m_epoch ) : 0; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. update from upstream instead. Also unnecessary, can't realistically happen until the 2038 bug hits.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just verified master and v0.13.1 still have the bare It's a profiler thing so I'll remove it. I could send the guard upstream to tracy separately later.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. upstream if possible then |
||
| assert( broadcastMsg.activeTime >= 0 ); | ||
| m_broadcast->Send( broadcastPort, &broadcastMsg, broadcastLen ); | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, this technically works around the crashes ig. Not sure this behaves correctly tho, and will still crash from lua (much rarer, can still happen).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you're right... it'll probably still crash.
The destructor disown covers the C++ side, but
CreateSoundContainerisadopt-bound. When async GC collects one it runs~SoundContaineron a threadpool worker whileAudioMan::Update()reads that channel'suserDataon the main thread. The null checks don't close the window since the container can get freed after the check passes. Also, the looping case looks wrong (disownedloop -1channels would keep playing with nothing left to stop them).I'll remove this from the PR rather than do a partial fix. It's out of scope for the determinism and cross-platform work I should be focusing on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah this is much better addressed in #265