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

Refactor Switches and Variables #1475

Merged
merged 2 commits into from Nov 4, 2018

Conversation

Projects
None yet
4 participants
@fmatthew5876
Copy link
Contributor

fmatthew5876 commented Nov 3, 2018

Depends: EasyRPG/liblcf#278

  • No more count fields
  • Variables are signed
  • Reads don't resize the array
  • Writes to id <= 0 silently do nothing.
  • Writes to any value N > 0, even > 5000 actually work. It writes a
    value which is saved to LSD and can be read back. This works in
    RPG_RT to enable any number of variables.
  • Matches RPG_RT behavior and written to LSD in same way

I've tested all these behaviors using ChangeVariable event commands and \v[N] in messages. I've also verified what gets written to LSD file when saved. Behavior matches RPG_RT.

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:swvar branch from 7f20a34 to a5b2b78 Nov 3, 2018

Refactor Switches and Variables
* No more count fields
* Variables are signed
* Reads don't resize the array
* Writes to id <= 0 silently do nothing.
* Writes to any value N > 0, even > 5000 actually work. It writes a
  value which is saved to LSD and can be read back. This works in
  RPG_RT to enable any number of variables.
* Matches RPG_RT behavior and written to LSD in same way
* Always clamp in Game_Variable::set()

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:swvar branch from a5b2b78 to 58bef92 Nov 3, 2018

@carstene1ns
Copy link
Member

carstene1ns left a comment

This looks really good to me.
I would suggest keeping some debug log when accessing invalid Switches/Variables, as this makes it easier to spot actual game bugs.

}
if (variable_id > vv.size()) {
vv.resize(variable_id);
}

This comment has been minimized.

@carstene1ns

carstene1ns Nov 4, 2018

Member

Removed comment: // Clamp to variable range

@Ghabry

This comment has been minimized.

Copy link
Member

Ghabry commented Nov 4, 2018

Thanks for the refactor, this also makes debugging read/writes of S/V easier as you don't need to use memory breakpoints anymore.

Writes to any value N > 0, even > 5000 actually work. It writes a
value which is saved to LSD and can be read back. This works in
RPG_RT to enable any number of variables.

This already worked before btw.

I'm okay with deleting the "resized array to X elements" message as it gave no advantages but the "index is invalid" message should be kept.

You also removed this (totally arbitrary) limit of PLAYER_VAR_LIMIT/1000000. Does RPG_RT really support writes up to 2^31? I'm worried about memory grow and gigantic savegames.

@Ghabry Ghabry added this to the 0.5.5 (or 0.6.0) -> we will see milestone Nov 4, 2018

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:swvar branch from 350d967 to 8f00c8b Nov 4, 2018

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Nov 4, 2018

I added back the debug logging. I agree its helpful for finding bugs.

You also removed this (totally arbitrary) limit of PLAYER_VAR_LIMIT/1000000. Does RPG_RT really support writes up to 2^31? I'm worried about memory grow and gigantic savegames.

Variables are already clamped to 999999(9) so game code won't be able to write past that anyway. I didn't test clamping on \v[N], but we don't resize the array on reads so it doesn't matter.

@fmatthew5876 fmatthew5876 force-pushed the fmatthew5876:swvar branch from 8f00c8b to 5ec72a4 Nov 4, 2018

@Ghabry

This comment has been minimized.

Copy link
Member

Ghabry commented Nov 4, 2018

This "report_limit" was btw added because Win Wind Windy used out-of-bounds variable writes for a replay function and this became so extremely spammy (up to 60 writes per second) that the framerate went to 0 on some platforms :/.

I made some testing with really high variables (via lcf2xml) and RPG_RT already gets problems around 100 million (Saving hangs without error?), with higher values it runs out of memory when writing the variable or saving.

Therefore there won't be any game using this but on the other hand this is also a really trivial way to crash our Player.

@fmatthew5876

This comment has been minimized.

Copy link
Contributor Author

fmatthew5876 commented Nov 4, 2018

4 * 9999999 is 40MB. With the clamping in Set() that's the worst damage a rogue rm2k3 ChangeVariable command could do. Not much on today's machines. Our current memory overhead on games like heroes realm is more than that.

The only way even more variables could get loaded into memory if someone hacked more into an LSD file. Crashing or performing badly on corrupted data seems a normal thing to do.

In the new Rpg makers I'm sure developers are free to allocate huge objects in RGSS and crash the system just the same.

I don't feel strongly either way. If you prefer to add additional checks I can do that. Otherwise I'm fine with it as is.

@Ghabry

This comment has been minimized.

Copy link
Member

Ghabry commented Nov 4, 2018

you are right, I forgot that using a manipulated LSD would work, too. making this check useless.

@Ghabry

Ghabry approved these changes Nov 4, 2018

@Ghabry

This comment has been minimized.

Copy link
Member

Ghabry commented Nov 4, 2018

Jenkins: Test this please :*

@Ghabry Ghabry merged commit 7d43de4 into EasyRPG:master Nov 4, 2018

6 checks passed

Android (armeabi-v7a) Build finished.
Details
GNU/Linux Build finished.
Details
OSX Build finished.
Details
Windows (x64) Build finished.
Details
Windows (x86) Build finished.
Details
web Build finished.
Details

@fmatthew5876 fmatthew5876 deleted the fmatthew5876:swvar branch Nov 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.