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.

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

Commit e9c7e03 breaks compatibility with Umineko Saku #17

Open
stephenmk opened this issue Sep 13, 2023 · 21 comments
Open

Commit e9c7e03 breaks compatibility with Umineko Saku #17

stephenmk opened this issue Sep 13, 2023 · 21 comments

Comments

@stephenmk
Copy link

All episodes of Umineko were ported to chronotrig's Ponscripter engine in 2019 as a part of the Umineko Saku compilation.

Commit e9c7e03 made changes to the getspsizeCommand function which broke compatibility with all of the Saku version scripts. These Saku scripts contain a function named mld which is used for loading and placing sprites on the screen. This function uses the getspsize command. Due to the changes in commit e9c7e03, this function incorrectly calculates the placements of sprites and often places them outside of the screen's view.

Below are some example screenshots. The screenshots on the right were taken using the latest version of the code in this repository. The screenshots on the left were taken using the same code but with the changes from commit e9c7e03 manually reverted.

1694645699256

1694646404256

1694646429256

@drojf
Copy link

drojf commented Sep 14, 2023

Can you send the first few commented lines of your game script? I'm just wondering whether you have "steam umineko" scaling enabled or "nscripter compatible" scaling enabled.

Eg if you have ;value2500,modew540@2x@umineko then it will use umineko steam scaling, but if you have ;value2500,modew540@2x then it will use nscripter comppatible upscaling, see d2b5d19

I think it would be best to add a third mode to the umineko scaling, specifically for this case, rather than trying to figure out what is wrong, which reverts this commit's changes. Something like ;value2500,modew540@2x@uminekosaku2019

@stephenmk
Copy link
Author

Here's the first 10 lines of each script.

Umineko

;value2500
;gameid umineko4_when_they_cry_4
*define
humanz 900

;windowback
automode
automode_time -50
mode_wave_demo

Umineko Chiru

;value2500
;gameid umineko8_when_they_cry_4
*define

;デバッグ用(セーブデータを別のフォルダに保存)
;savedir "savedata"
;effectskip 0
humanz 900

;windowback

Tsubasa

;value2500
;gameid uminekoTsubasa_when_they_cry_4
*define

humanz 900

;windowback
automode
automode_time -50
mode_wave_demo

Hane

;value2500
;gameid uminekoHane_when_they_cry_4

*define

humanz 900

;windowback
automode
automode_time -50

Saku

;value2500
;gameid uminekoSaku_when_they_cry_4

*define

humanz 900

;windowback
automode
automode_time -50

07th theater

;value 2500
;gameid 07th_theater_when_they_cry_4
*define

;デバッグ用(セーブデータを別のフォルダに保存)
;savedir "savedata"
;effectskip 0
humanz 900

;windowback

@drojf
Copy link

drojf commented Sep 14, 2023

I think the issue is that the default multiplier style is UMINEKO (for Umineko steam release), even if you haven't specified any scaling options at all...

multiplier_style = UMINEKO;

Can you try changing that line to

    multiplier_style = FULL;

And seeing if it fixes the problem (leave the game scripts as-is)?

@stephenmk
Copy link
Author

stephenmk commented Sep 14, 2023

Yes, that works. I recompiled using the latest code and only that one change (FULL instead of UMINEKO) and the problem seems to be fixed.

I wonder why it wasn't being set correctly here:

if (!strncmp(buf, "@umineko", 8)) {
buf += 8;
multiplier_style = UMINEKO;
} else {
multiplier_style = FULL;
}

@drojf
Copy link

drojf commented Sep 14, 2023

The reason is, that part only runs if you've supplied the mode argument (eg. ;value2500,modew540@2x@umineko), if you check the surrounding if statement.

@drojf
Copy link

drojf commented Sep 14, 2023

@TellowKrinkle Was the intended default scaling mode meant to be FULL scaling? Currently the default scaling mode is UMINEKO (for umineko steam).

@drojf
Copy link

drojf commented Sep 14, 2023

Another possible solution if we want to keep exactly the existing engine behaviour.


I'm wondering if it's just safer to just add another scaling option which only reverts that one commit, and have that as the default. That would keep the behavior the same as before. But not sure if this is the correct thing to do.

Eg:

  • Don't specify any scaling option = use Umineko scaling mode, with e9c7e03 disabled
  • Specify scaling, but don't specify (steam) umineko = use FULL scaling mode with e9c7e03 enabled
  • Specify scaling, and specify (steam) umineko = use Umineko scaling mode, with e9c7e03 enabled

@TellowKrinkle
Copy link
Member

TellowKrinkle commented Sep 14, 2023

@TellowKrinkle Was the intended default scaling mode meant to be FULL scaling? Currently the default scaling mode is UMINEKO (for umineko steam).

The intention was to work as a drop-in replacement for the umineko steam games, for people who wanted to use the non-widescreen versions of our mod, which is why all the defaults are for that.

Obviously it's not possible to be a drop-in replacement for all games, as many specify the exact same (lack of a) mode line as Steam Umineko. It's been a while, but IIRC it should already do what you mentioned, umineko scaling is only used by default if you don't have a mode setting at all, any other mode option will use normal scaling modes.

@TellowKrinkle
Copy link
Member

To be clear, this line should get you the same settings as a non-umineko ponscripter

;value2500,mode640@2x

@drojf
Copy link

drojf commented Sep 14, 2023

OK, if that's the case, then I think I'll resolve the issue by

  1. Make more clear on the readme.md how you should use this engine - basically that if you want to use it on anything other than unmodded Umineko steam / 07th-mod Umineko, you MUST at least consider what scaling options to use, and modify the first line of your script accordingly
  2. For the particular issue OP is having, no change to engine, just require OP to modify the top line of their scripts to what you just posted (;value2500,mode640@2x)

@TellowKrinkle
Copy link
Member

TellowKrinkle commented Sep 14, 2023

Oh wait, you want to add a third mode that toggles e9c7e03 alone? I don't think that's necessary
The old ponscriptor-fork branch did what is now called "FULL"
Umineko was based on some even older version of that, which was missing scaling in a few places
e9c7e03 is just us finding another place that Umineko was missing

@TellowKrinkle
Copy link
Member

If you do want to make sure, IIRC the other thing umineko mode affects is font size, which manifested as the wrong font size in load/save dialogs

@drojf
Copy link

drojf commented Sep 14, 2023

after reading what you've written, I agree, I don't think it is necessary.

I guess I just mentioned that as an option because I didn't feel confident about this stuff, and that would be the only way to ensure everything worked exactly the same as before the commit I made, for people who did not specify a mode line. But it looks like you've confirmed what I wasn't sure of.

@stephenmk
Copy link
Author

stephenmk commented Sep 14, 2023

For the particular issue OP is having, no change to engine, just require OP to modify the top line of their scripts to what you just posted (;value2500,mode640@2x)

Yes, this seems to work fine. Thanks to you both for the help.

For what it's worth, I think it would be nicer to have this fork function as the original ponscripter by default (at least as much as possible). A lot of people aren't savvy enough to decode and edit these scripts to add the necessary mode line. It seems this fork is the de facto main version of ponscripter at the moment and is presented as such e.g. in the arch linux AUR build script; it's not presented as the "drop-in replacement for Umineko on Steam" version of ponscripter.

Since you guys control the distribution of the drop-in replacement binaries for the Steam version of Umineko, I think you could just set the necessary flag during the compile time of those executables rather than hard-coding it here.

Anyway, that's just my two cents. I can understand why you may not be interested maintaining that sort of backwards compatibility.

@drojf
Copy link

drojf commented Sep 14, 2023

@TellowKrinkle - Just so you know, all our Umineko mod scripts already have the ;value2500,modew540@2x@umineko line (or whatever variant is needed).

So if "original ponscripter" were the default, it would only affect unmodded Umineko Steam, which doesn't have the mode commands.

@TellowKrinkle
Copy link
Member

Yeah it's probably fine to change the default
I mostly set it that way because I wasn't aware of any other ponscripter projects at the time (just Umineko and Ciconia, which uses w720)

@stephenmk
Copy link
Author

stephenmk commented Sep 14, 2023

The ports of Higurashi to ponscripter released in the Hou+ compilation also don't have the mode parameter specified, but I haven't checked to see if they're affected by the multiplier issue.

Edit: There are indeed instances of the issue in Hou+ although they're not as numerous as in the Umineko Saku versions.

1694673953257

@drojf
Copy link

drojf commented Sep 15, 2023

OK , I tried making a PR to both make nscripter scaling mode the default, and to document the scaling options/what the default scaling is: #18

@drojf
Copy link

drojf commented Sep 29, 2023

OK, I've published a new release v4.0.0. @stephenmk if you can confirm it's working as expected, I can close this issue.

https://github.com/07th-mod/ponscripter-fork/releases/tag/v4.0.0

@stephenmk
Copy link
Author

Looks good. Thank you again.

I tried Saku with two different parameter lines, ;value2500 and ;value2500,mode640@2x, and they both worked correctly as expected. Adding @umineko also reproduced the error as expected.

By the way, I checked the --version option from the command line and noticed it's completely out of date. Might be worthwhile to update it if it's not much effort.

$ ponscr --version
System info: Intel CPU, with functions: MMX SSE SSE2 SSSE3 
Ponscripter version 20160615 '' (NScr 2.81)
Based on ONScripter by Ogapee <ogapee@aqua.dti2.ne.jp>
Currently maintained by "Uncle" Mion Sonozaki <UncleMion@gmail.com>

Copyright (c) 2001-2011 Ogapee, 2006-2011 insani, Haeleth, Sonozaki et al.
This is free software; see the source for copying conditions.

@drojf
Copy link

drojf commented Sep 29, 2023

ah thanks for checking all that.

Yea I'll see if I can update the version text...

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

No branches or pull requests

3 participants