Skip to content

Add confirmation to the SHIFT-Q shortcut before closing the game - #22

Merged
drojf merged 2 commits into
07th-mod:modfrom
I-Have-No-Idea-What-IAmDoing:mod
Jun 27, 2025
Merged

Add confirmation to the SHIFT-Q shortcut before closing the game#22
drojf merged 2 commits into
07th-mod:modfrom
I-Have-No-Idea-What-IAmDoing:mod

Conversation

@I-Have-No-Idea-What-IAmDoing

Copy link
Copy Markdown

Fixes #19


I decided to add the confirmation instead of outright disabling it since this seems to be the easier option and seems to be more consistent

This was a simple copy and paste job so I am not sure if this should be merged honestly...

I am not entirely sure if this works cause I can't build it on my machine in WSL as my filesystem has a space in it which is not escaped by the makefile and it took a bit more effort than I was willing to put in to fix that.

@drojf

drojf commented Jun 19, 2025

Copy link
Copy Markdown

I am not entirely sure if this works cause I can't build it on my machine in WSL as my filesystem has a space in it which is not escaped by the makefile and it took a bit more effort than I was willing to put in to fix that.

I haven't looked at the changes yet, but I could make it build on github actions and then we can both try it out

@I-Have-No-Idea-What-IAmDoing

Copy link
Copy Markdown
Author

The failing builds seems to just be warnings from the external libraries that can be ignored

@drojf

drojf commented Jun 20, 2025

Copy link
Copy Markdown

I'm not sure about the failing builds - the error is when running .travis_before_install.sh it returns 255, but not sure if it is from choclatey or from the script itself (since choclatey doesn't emit any errors?)

image

@I-Have-No-Idea-What-IAmDoing

I-Have-No-Idea-What-IAmDoing commented Jun 20, 2025

Copy link
Copy Markdown
Author

It might be an error with docker based off of the workflow file?

@drojf

drojf commented Jun 21, 2025

Copy link
Copy Markdown

If it's too difficult then don't worry about it, I'll ask tellowkrinkle it...also, I think I can't run it on your branch, because there's some stuff which is private to this repo which is necessary for the build

@drojf

drojf commented Jun 21, 2025

Copy link
Copy Markdown

The ubuntu builds succeeded just now, see https://github.com/07th-mod/ponscripter-fork/actions/runs/15790116056 (I guess maybe they always succeeded, it's just Github Actions was setup to cancel all jobs if one job failed). I guess if you have a linux machine you can test it.

For the Windows-2022 (no steam) build, it fails because .configure thinks gcc doesn't work (when testing/configuring jpeg-6bI think)

loading site script /etc/config.site
checking for gcc... gcc
checking whether the C compiler (gcc -I/d/a/ponscripter-fork/ponscripter-fork/src/extlib/include  -L/d/a/ponscripter-fork/ponscripter-fork/src/extlib/lib ) works... gcc -O -I/d/a/ponscripter-fork/ponscripter-fork/src/extlib/include   -c -o uncompr.o uncompr.c
no
configure: error: installation or configuration problem: C compiler cannot create executables.
make[1]: *** [Makefile.extlibs:134: extlib/src/jpeg-6b/Makefile] Error 1
make[1]: *** Waiting for unfinished jobs....

I'm not sure how you normally get the .configure log file or make .configure print the error message (for the gcc -O -I/d/a/ponscripter-fork/ponscripter-fork/src/extlib/include -c -o uncompr.o uncompr.c command) though...

Also, the windows-2022-steam fails with a different error, but if at least the non-steam version worked, we could test it on windows.

Edit: I think some of the windows builds are multitheraded, so the output log may be out-of-order.

@I-Have-No-Idea-What-IAmDoing

Copy link
Copy Markdown
Author

it seems that strategy.fail-fast defaults to true so that causes the others to be cancelled...

@drojf

drojf commented Jun 21, 2025

Copy link
Copy Markdown

ah sorry, I already applied some fixes to my branch, but forgot to apply it to yours

@drojf

drojf commented Jun 21, 2025

Copy link
Copy Markdown

@TellowKrinkle I'm not really sure why the builds are failing...

You can see what we tried on https://github.com/07th-mod/ponscripter-fork/tree/noidea_trial_build

The errors don't appear to have anything to do with the changes made to the engine code, but to be safe I would start from the mod branch in case we inadvertently broke something, and just try to get the existing code to build again.

@TellowKrinkle

Copy link
Copy Markdown
Member

The steam builds are probably failing because the steam SDK we built them against was downloaded from our server, which doesn't exist anymore.

The Mac non-steam build is probably failing because one of the dependencies doesn't build properly on the updated compiler.

Not sure about the windows non-steam build.

@drojf

drojf commented Jun 21, 2025

Copy link
Copy Markdown

Oh...that explains the steam builds

I don't think I have the expertise to fix any makefile issues, so I'll leave it for you, if you want to look at it later

@TellowKrinkle

Copy link
Copy Markdown
Member

drojf added a commit that referenced this pull request Jun 21, 2025
@drojf

drojf commented Jun 21, 2025

Copy link
Copy Markdown

Yes, that fixed the (non steam) macOS build.

Note: Added your patch in this commit: 50627c4

@I-Have-No-Idea-What-IAmDoing

Copy link
Copy Markdown
Author

It says it needs to workflow awaiting approval on my end

drojf added a commit to I-Have-No-Idea-What-IAmDoing/ponscripter-fork that referenced this pull request Jun 23, 2025
@drojf

drojf commented Jun 23, 2025

Copy link
Copy Markdown

@I-Have-No-Idea-What-IAmDoing uh sorry, I forgot to check in on what I was doing

So on the weekend I tried using ChatGPT/ai assistance for the first time, since I thought it would help in this case where I don't have much expertise in this field, which did actually help, and it turned out some of the issues were just the upgrade to GCC 15, which is more strict with what is considered an error. So I went in and either fixed the error or made it ignore the error. But I didn't have time to go through all the errors yet.

The other issue (which I'm not 100% is true) is that there seems to be some build difference between when I run builds on this PR (https://github.com/I-Have-No-Idea-What-IAmDoing/ponscripter-fork on the 'mod' branch), and when I run builds on the main repo (I use this branch: https://github.com/07th-mod/ponscripter-fork/tree/noidea_trial_build), so for now I've just been running the builds on the main repo. But then, it doesn't notify you when I do any work on the main repo...

I'll try applying the changes to the PR and see if the build turns out the same as when I apply the changes to the main repo.

@drojf

drojf commented Jun 23, 2025

Copy link
Copy Markdown

I did a test just now and I get the same error on Windows (no steam) build, so I assume the build result is more or less the same as building on the main repo.

So I'll push my changes to this PR branch so you can see what is going on.

@I-Have-No-Idea-What-IAmDoing

I-Have-No-Idea-What-IAmDoing commented Jun 24, 2025

Copy link
Copy Markdown
Author

I am pretty sure sdl_image lib can't find 'png.h' which is in 'libpng-1.6.2' folder of it's own external lib.

Not sure why only windows is having that problem...

Looking at the ubuntu one it seems that it missing these includes? But I think we don't need one of them (also need to translate it into window paths of course)
"-I/pons/src/extlib/include -I/pons/src/extlib/include/libpng12 -I/pons/src/extlib/include"

@drojf

drojf commented Jun 24, 2025

Copy link
Copy Markdown

The build works now, and your change works as expected. I also briefly tested whether the build had any issues by skipping through umineko opening, and it seems OK, even the video playback.

Here is the built executable for windows: ponscr-windows.zip

To make things neat, I'll put all my changes related to the build system on another PR.

Once I've done that I'll accept your PR since it seems to work as expected.

Later I'll tidy up the other build-system fixes PR, check I haven't done anything insane etc.

@I-Have-No-Idea-What-IAmDoing

I-Have-No-Idea-What-IAmDoing commented Jun 25, 2025

Copy link
Copy Markdown
Author

Thanks for all the help on getting this to actually build!

I tested it myself and it seems to work on my machine on Umineko: Questions Arc, so I think it didn't break anything :)

You should also put somewhere in the build instruction about the 'space in path' thing to let other who try building from source know

@TellowKrinkle

Copy link
Copy Markdown
Member

BTW I got the windows build working in https://github.com/07th-mod/ponscripter-fork/tree/refs/heads/noidea_trial_build by updating libpng and libjpeg to their latest versions plus a modification to sdlmixer.

@drojf

drojf commented Jun 27, 2025

Copy link
Copy Markdown

oh ok, in that case I should delete my PR here, since it doesn't fix the builds properly: #23

I had figured upgrading the libs might be the correct way to fix it, the things you fixed seemed to be exactly the same things I had to tell the build system to not check/forcibly make them build....

I was worried that upgrading libraries might break something with the engine, but I didn't really know. I'm assuming if you made this change in your experience it wouldn't break anything?

I'm also assuming the updated libs will now apply to all operating system builds, is that correct? (EDIT: uh I guess the linux + non-steam may or may not use internal jpeg/png so may or may not be affected? Looking at .travis_run.sh.)

@drojf

drojf commented Jun 27, 2025

Copy link
Copy Markdown

@TellowKrinkle

For the steam SDKs which used to be on our server, I thought I had a backup, but it looks like I only backed up the public files on the server, so I don't have it. If it can't be re-downloaded from the Steam Developer website, and you don't have a backup, you could ask Italo if he still has a backup of the server.

I'm not really fussed if it doesn't work as the Umineko .exes aren't used very often / it's up to you if you want to fix it. I guess it's useful for the Steam version of Ciconia.

@drojf
drojf merged commit ed0d886 into 07th-mod:mod Jun 27, 2025
0 of 21 checks passed
@drojf

drojf commented Jun 27, 2025

Copy link
Copy Markdown

I've accepted the original PR.

I made a new PR to organize the commits related to the build system (I noticed the noidea_trial_build may need some tidying up).

Please continue the discussion in #24

@I-Have-No-Idea-What-IAmDoing

Copy link
Copy Markdown
Author

Let's go thanks for all the help, can't wait to finish the answer arc now lol

@TellowKrinkle

Copy link
Copy Markdown
Member

You can download the SDK from Steam's website after signing up for their partner program, that's how I got it originally.
The main question is how to supply it to the CI, as it isn't licensed for redistribution (which is why I previously had it download it from our server over SSH).

@drojf

drojf commented Jun 28, 2025

Copy link
Copy Markdown

so...probably not a long term solution, but there's a way to access OneDrive files programmatically, so I've hosted the files there for now (the link to it is in a github secret). I also updated the build script to not use SSH anymore.

The archive has the 141, 134, and 129a sdk .zip files inside, but I can update it at any time if there is some other file we need.

The MacOS and Windows steam builds now work, but the linux one fails with

Now run "make" to install steam for ponscripter.
make[2]: *** No rule to make target `redistributable_bin/linux64/libsteam_api.so', needed by `../../lib/libsteam_api.so'.  Stop.

I would double check that I have included the files correctly for the linux build, in case I messed something up.

@TellowKrinkle

Copy link
Copy Markdown
Member

The issue with linux is it runs the script under docker, which doesn't have csharp or python installed
To avoid having to deal with getting csharp into the docker image, I moved the steam API to an HTTP download from my own web server.

@drojf

drojf commented Jun 28, 2025

Copy link
Copy Markdown

Anymore changes? Otherwise I'll tidy up the branch with the build fixes and merge

@TellowKrinkle

Copy link
Copy Markdown
Member

No that's it

drojf added a commit that referenced this pull request Jun 29, 2025
drojf added a commit that referenced this pull request Jun 29, 2025
drojf added a commit that referenced this pull request Jun 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Disable or add confirmation to SHIFT-Q shortcut to close the game

3 participants