07th-mod / ponscripter-fork

Fork of the Ponscripter visual novel engine to take advantage of SDL2 and improve Steam integration
GNU General Public License v2.0
15 stars 4 forks source link

Commit e9c7e03 breaks compatibility with Umineko Saku #17

Open stephenmk opened 1 year ago

stephenmk commented 1 year ago

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 commented 1 year ago

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 https://github.com/07th-mod/ponscripter-fork/commit/d2b5d190477c0b32bd0aca87972a5c047c1ef451

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 commented 1 year ago

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 commented 1 year ago

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...

https://github.com/07th-mod/ponscripter-fork/blob/d2b5d190477c0b32bd0aca87972a5c047c1ef451/src/ScriptHandler.cpp#L72

Can you try changing that line to

    multiplier_style = FULL;

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

stephenmk commented 1 year ago

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:

https://github.com/07th-mod/ponscripter-fork/blob/d2b5d190477c0b32bd0aca87972a5c047c1ef451/src/ScriptHandler.cpp#L1039-L1044

drojf commented 1 year ago

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 commented 1 year ago

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

drojf commented 1 year ago

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:

TellowKrinkle commented 1 year ago

@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 commented 1 year ago

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

;value2500,mode640@2x
drojf commented 1 year ago

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 commented 1 year ago

Oh wait, you want to add a third mode that toggles e9c7e0394160e6cc4e5dbc9f983bd6f3b15d4075 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 e9c7e0394160e6cc4e5dbc9f983bd6f3b15d4075 is just us finding another place that Umineko was missing

TellowKrinkle commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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: https://github.com/07th-mod/ponscripter-fork/pull/18

drojf commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

ah thanks for checking all that.

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