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

liblcf should not skip empty arrays #229

Closed
fdelapena opened this Issue Jul 24, 2018 · 21 comments

Comments

Projects
None yet
5 participants
@fdelapena
Copy link
Contributor

fdelapena commented Jul 24, 2018

This issue makes liblcf remove chunks which should be saved empty.
This affects starting party and may be related with an old menu commands issue.
Changing this behaviour may require updating stuff in Player.

@fdelapena fdelapena added the Chunks label Jul 24, 2018

@20kdc

This comment has been minimized.

Copy link

20kdc commented Jul 24, 2018

I don't think fixing this will directly impact Player (saving empty arrays can only make things more correct, short of optional arrays that we don't know about), but giving the correct default values, which I think are:

party: length 1, contents: [1]
menu commands: length 1, contents: [0]

could cause conflict with the 2k3 detection heuristics. The second might, in any case.

@20kdc

This comment has been minimized.

Copy link

20kdc commented Jul 24, 2018

...ok, so:

The current behavior of not saving empty arrays that are also the default value is probably correct.

Unfortunately, there are a few arrays which should have default values which don't, and are thus not saved because their default values are empty.

In particular: Chipset's 3 arrays, System party, and System menu_commands (those last two I noted above: They don't have default value descriptions in the CSV.)

generator.py right now doesn't support default values with arrays in, so here's my shot at it.
https://gist.github.com/20kdc/7b131f083317bbf19c291893092042c5

Tested to compile, but IDK what kind of havoc it causes when running.
Probably at least a little on .flow with pseudo2k3 detection probably being fried.

Also:
System,party,f,Vector<Int16>,0x16,[1],Array - Short
System,menu_commands,f,Vector<Int16>,0x1B,[0],Array - Short - RPG2003

@Ghabry

This comment has been minimized.

Copy link
Member

Ghabry commented Jul 27, 2018

good start @20kdc but it will probably also need changes in reader_struct for the "IsDefault" handling. :/

And some lines of code in the Setup() funcs can be removed.

@fmatthew5876

This comment has been minimized.

Copy link
Contributor

fmatthew5876 commented Sep 25, 2018

could cause conflict with the 2k3 detection heuristics. The second might, in any case.

We just need to check whether the DB is 2k or 2k3. If its 2k, we should never write any of the 2k3 arrays.

Changing this behaviour may require updating stuff in Player.

I don't think so. The player can already load ldb's generated by RPG_RT.exe which have the empty arrays. All this does is make it more compatible.

@carstene1ns

This comment has been minimized.

Copy link
Member

carstene1ns commented Sep 25, 2018

...and then some smart person comes and edits some 2k game in 2k3 - hello .flow.

@Ghabry

This comment has been minimized.

Copy link
Member

Ghabry commented Sep 25, 2018

c1 is talking about this: https://github.com/EasyRPG/Player/blob/master/src/player.cpp#L656

(.flow was modified to 2k3 DB but uses a 2k RPG_RT). Don't think any of your changes could break this heuristic.

@fmatthew5876

This comment has been minimized.

Copy link
Contributor

fmatthew5876 commented Sep 25, 2018

Wow.. Never underestimate your users.. hah

Does anyone know why they did this for .flow? Do some 2k3 features secretly work within 2k's RPG_RT.exe?

@Ghabry

This comment has been minimized.

Copy link
Member

Ghabry commented Sep 25, 2018

I think the one who translated .flow simply messed up (or had no RPG maker 2000 editor around? I have no idea).

At least in EasyRPG it breaks because RPG Maker 2003 allows configuring which menu items appear in the Menu scene which resulted in an empty menu screen but RPG_RT of 2000 obviously doesn't care >.>

@fdelapena

This comment has been minimized.

Copy link
Contributor Author

fdelapena commented Sep 25, 2018

RPG Maker 2003 allows to edit RPG Maker 2000 games. Old 2003 versions ( <= 1.09a) allow to open 2000 games without any warning prompt. Some chunks get added, not sure if per-tab or per-modified block. If I recall correctly, 2003 1.10+ already warns about this.

There is a well known trick to learn how editor modifies the ldb. Just create a new project, then remove the .ldb file. Open the project, go to database and press OK. A new "empty" bare bones ldb (around 3 kb) will be created. Not even title, system or vehicle are set (and yes, there are some games out there which have some of them unset!).

With a simple test ldb gets easier to figure out what gets modified between editor versions if we ever want to prevent version data clashes, as some games are created by multiple authors. This is, however, a real corner case and probably not worth until some broken game gets reported. But still useful to set proper liblcf defaults when some blocks are unset.

@fmatthew5876

This comment has been minimized.

Copy link
Contributor

fmatthew5876 commented Sep 26, 2018

The more I study this, the more bizarre it gets.

Skill type and scope are enums, not array but they are always written as 0x080100 and 0x0C0100 when defaulted.

Why there is a 1 in there I really don't know.

@fmatthew5876

This comment has been minimized.

Copy link
Contributor

fmatthew5876 commented Sep 26, 2018

I've got a WIP on solving this here: https://github.com/fmatthew5876/liblcf/tree/ldb_empty_array

The way I do it is I add 2 new columns to fields.csv

  • emptyhandler - what to do if the field is defaulted
  • is2k3 - if the field is 2k3 specific

EmptyHandler has the following values so far:
0:
1: ID,0
2: ID,1,0

As mentioned above, some non-array fields still have some special default behavior. So the emptyhandler is not restricted to array types.

The is2k3 flag is needed to avoid invoking the emptyhandler if it's a 2k3 field but we're a 2k db.

generate.py, Field types, and LCF macros are updated to handled these 2 new attributes.

The following changes to reader_struct

  • Check the is2k3 flag and don't write the field at all if it's 2k3 specific but this is a 2k database. Incidentally this fixes a bug where Terrain::footstep (2k3 only sound effect) was always being written. May be others like this fixed as well.
  • If the field is defaulted, check the emptyhandler value and do as described above. lcf size is also supported.

Comments welcome. I'm sure people here know something I don't and there is maybe a better way to go about this.

@fdelapena

This comment has been minimized.

Copy link
Contributor Author

fdelapena commented Sep 26, 2018

If I recall correctly, 01 is the size of the of the next block (1 byte), which value is 00. In case of an array, I guess that may mean empty array.

Note: Borland Delphi integer values above 0x7F, it uses the next byte (known as TLV or VLQ, variable length). This also may apply for large arrays, just in case. liblcf has some stuff to habdle 1d/2d arrays, single integers and such.

@fmatthew5876

This comment has been minimized.

Copy link
Contributor

fmatthew5876 commented Sep 27, 2018

If I recall correctly, 01 is the size of the of the next block (1 byte), which value is 00. In case of an array, I guess that may mean empty array.

Why do you need to specify a size for a single enum value. It's always 1 byte. Or if it was more than 1 byte it could use the multibyte encoding you mentioned.

@fdelapena

This comment has been minimized.

Copy link
Contributor Author

fdelapena commented Sep 27, 2018

It seems that's the way Borland Delphi objects are stored. Strings are not null delimited, they also use a size prefix, so it seems even when the value is empty (defined, but null) the value is 0 and prefixed with 01 which means the size length (bytes used for the data, even if 00, which spends 1 byte).

@Ghabry

This comment has been minimized.

Copy link
Member

Ghabry commented Sep 27, 2018

reader_struct intensifies

Just took a quick look at your branch, the change looks good fine to me. 👍

Style notes: We use tabs instead of spaces, add whitespace after // (//comment -> // comment)

@fmatthew5876

This comment has been minimized.

Copy link
Contributor

fmatthew5876 commented Sep 27, 2018

I think a better approach is to define a type system. Each type will have it's specific behavior for defaults and everything else. I will do them like I did with flags.

This should result in a more robust solution.

@fmatthew5876

This comment has been minimized.

Copy link
Contributor

fmatthew5876 commented Sep 29, 2018

Why do size fields exist?

The size field tells you the size of the next array. Then the array itself has the size encoded again. It doesn't make any sense.

@fdelapena

This comment has been minimized.

Copy link
Contributor Author

fdelapena commented Sep 29, 2018

LCF array structures are weird. For example, 2D array type contains a 1D array, and then inside even might contain again the size for variable length integers, which may take more than one byte, so I guess the mostly nonsense layered nested size is possible even with this structure, as every data type may take any unlimited length this way. This implementation also shows somehow how they are working too.

@fmatthew5876

This comment has been minimized.

Copy link
Contributor

fmatthew5876 commented Sep 29, 2018

Is there some specific reason why you guys don't use C++ enums for the enum types in rpg structs? The code defines enums but the actual members are ints.

@Ghabry

This comment has been minimized.

Copy link
Member

Ghabry commented Sep 29, 2018

I'm not aware of a specific reason, you will just need to fix all the code afterwards because this causes compiler errors :D.
The size fields are really useless, at least for the Player but RPG_RT will report a stream error when they are wrong :/. Also what the size field contains differs, sometimes it's byte, sometimes it's amount of values.

@fmatthew5876

This comment has been minimized.

Copy link
Contributor

fmatthew5876 commented Sep 30, 2018

Some weird trivia:

We have size fields. But we also have count fields. In RPG::System, the party and menu size fields actually count the number of elements, not the number of bytes.

Skill::battler_animation has unique behavior. It's valid values are 0 to actors.size()-1. It looks like it's only used in the gui. If it's not present in the ldb, that means the gui box is blacked out. Otherwise if it's >= 0 it represents one actor. I changed this one to default to -1 in liblcf to represent no actor.

There is one field that is rm2k specific. That is RPG::Terms 0x3 Escape Success. All Terms strings are written as empty strings, but Rm2k3 will not write this field the ldb file if it's empty as it's not used in rm2k3.

So far I've got both an empty game and the default project in rm2k3 giving a bit perfect match when read and written using liblcf.

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.