Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Megashot Fix #174

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Megashot Fix #174

wants to merge 10 commits into from

Conversation

realSquidCoder
Copy link
Contributor

@realSquidCoder realSquidCoder commented Jan 25, 2025 •
edited
Loading

Revert #102 as its not needed anymore

@realSquidCoder
Megashot Fix
96be99b 
Close Stonesense instead of leaving it unresponsive after taking megashot.
@realSquidCoder
Update changelog.txt
5ecfd78
@realSquidCoder
whitespace
80bc07a
@realSquidCoder realSquidCoder marked this pull request as ready for review January 25, 2025 21:35
@NicksWorld
Copy link
Contributor

It is my understanding initial bug that lead to #102 has been fixed in #140. The only assert I could see possibly being hit was ensuring bitmap drawing wasn't held, which likely was caused by the extraneous call removed in #140.

If this is indeed the case, stonesense no longer has a need to become unresponsive following a megashot. The only change that should be needed, outside of reverting #102, is to backup lift_segment_offscreen_x/y and restore at the end of the function.

@myk002
Copy link
Member

myk002 commented Jan 28, 2025

Agree with NicksWorld
realSquidCoder, could you try reverting the logic changes in https://github.com/DFHack/stonesense/pull/102/files and verifying that this issue still exists?

@realSquidCoder
Copy link
Contributor Author

the crash is gone and a new issue has arisen in its stead: the map no longer gets drawn
image

I have confirmed that it still responds and tracks df etc

@realSquidCoder
Copy link
Contributor Author

now works as intended

NicksWorld
NicksWorld reviewed Jan 29, 2025
View reviewed changes
GUI.cpp Outdated Show resolved Hide resolved
main.cpp Outdated Show resolved Hide resolved
@realSquidCoder
See Description
eb8a0f6 
Removed debug code i forgot
added clockToMS to common header for future uses
megashot timer uses MS now
@realSquidCoder
Fix typos
24ef5a1
myk002
myk002 reviewed Jan 30, 2025
View reviewed changes
StonesenseState.h Outdated
@@ -27,6 +27,7 @@ class StonesenseState
FrameTimers stoneSenseTimers;
bool timeToReloadSegment;
bool timeToReloadConfig;
bool timeToCloseWindow;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

leftover?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, new var for debugging and future use.

Copy link
Member

@myk002 myk002 Feb 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can keep it in a local branch, but as it is, this is just confusing to have in the mainline code

It is not uncommon to have debugging changes in a local branch that you can merge into your local code as needed.

main.cpp Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NicksWorld NicksWorld NicksWorld left review comments

@myk002 myk002 myk002 left review comments

Assignees
No one assigned
Labels
None yet
Projects
51.04-r2
Status: Todo
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@realSquidCoder @NicksWorld @myk002