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

Change: Implement GRFv9 #357

Closed
wants to merge 2 commits into from
Closed

Change: Implement GRFv9 #357

wants to merge 2 commits into from

Conversation

PeterN
Copy link
Member

@PeterN PeterN commented Jan 12, 2025

We keep listing changes to the GRF spec and saying "that can come in GRFv9".

Well, here is an attempt to implement some changes, and call it GRFv9.

This is of course just a draft and is subject to improvements (or being scrapped...)

Action 6 changes are very... temperamental.

See OpenTTD/OpenTTD#13309

for sound in self.sound_list:
sound.write(file, 1)
sound.write(file, 2)
file.newline()

def get_size(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get_size needs adjusting. See frosch123@7ba80f2
(triggered by FIRS 5.0.0-beta-3

@frosch123
Copy link
Member

frosch123 commented Jan 14, 2025

I tested 3 variants of self-describing Action0 with property sizes: frosch123@d323112

  • Encode size as DWORD in front of every property data.
  • Encode size as BYTE for sizes < 0x100. Use byte 00 + DWORD for larger property data.
  • Encode size as SimpleGamma (same as in OpenTTD savegames).

As benchmark I used FIRS 5.0.0-beta-3: a large NewGRF with decent NFO size, and not as much sprites as vehicle sets.

Version Size of firs.grf delta
GRF v8 8909768
GRF v9 without size 9120401 GRF v8 + 210633
GRF v9 + dword per prop 9166481 GRF v9 + 46080
GRF v9 + byte+dword 9132145 GRF v9 + 11744
GRF v9 + SimpleGamma 9132007 GRF v9 + 11606

Opinion:

  • I think the file size-increment is negible compared to the size-increment of GRF v9 in general, and even less compared to the whole file size.
  • SimpleGamma is obviously the best, but the simpler byte+dword is not much worse. So, if SimpleGamma is "too complicated" to document, then byte+dword is good enough.

@rubidium42
Copy link
Contributor

I tested 3 variants of self-describing Action0 with property sizes: frosch123@d323112

Has thought been put into what we're going to do with GRFCodec and friends?

  • Does the author of the NFO need to manually count the bytes and encode this?
  • Might something be possible with an escape sequence that is to be replaced by the count? Would NFO-renum or GRFCodec be doing that?
  • Would decoding restore the escape sequences?
  • Would GRFCodec just never support GRFv9?

@PeterN
Copy link
Member Author

PeterN commented Jan 14, 2025

While the overhead is low for NML which nearly always only writes properties for one item at a time, any outputter in which property "batching" is used with num-info > 1 will see a larger relative increase in size. Not necessarily a problem, just pointing it out.

This is common with hand-written NFO (and grf-py can do it), and asking NFO authors to hand-encode every property size is awkward.

Variably-sized size information will also affect writing correct Action 6 offsets.

@PeterN
Copy link
Member Author

PeterN commented Jan 14, 2025

I tested with the property size tweak. It puts the property size before the property number, which wasn't what I expected, as it means the value changes depending on my items are being changed.

@PeterN PeterN force-pushed the grfv9 branch 2 times, most recently from 8bf52e1 to fc7b4be Compare January 16, 2025 20:33
@rubidium42
Copy link
Contributor

Would it be an idea to not go fully self-describing on Action0, but introduce a new action that defines a basic description of the actions. However, any property with a variable length must define their length in the Action0, but any property without length does not.

Some of these variable length properties already have a size, e.g. airport layouts (swap num layouts and size fields though), or never/always refittable cargoes. However, these have a different size of their size field, so the describing new action should also define the length of the size field for variable size properties.

By designing it as a separate action, we don't need to duplicate this information for each and every property. It also makes it easier to just skip it.
I would suggest adding this self-descriptive action at the end of the file, i.e. make it the last sprite. So it's potentially relatively easy to find? With NML you can also fill the table while building the NewGRF, so you only need to emit what you actually used. However, that will be tricky when combining multiple NML-generated NFOs into a single NewGRF. There one might consider amending GRFcodec to just merge these descriptions when there are multiple of these sprites in the combined source NFO.

@PeterN
Copy link
Member Author

PeterN commented Jan 16, 2025

I guess if you need to scan the whole NewGRF to find the property descriptor blocks you can probably cope with

  1. reading the Action 08 early so that the whole file can be GRFv9
  2. reading multiple descriptor blocks and treat them as one, so that the NML → NFO → combined NFO → grfcodec workflow doesn't need to do anything special. (A bit wasteful but works)

@PeterN PeterN closed this Jan 17, 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.

3 participants