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: skip realsprite palette validation step iff output is nfo AND… #254

Conversation

andythenorth
Copy link
Contributor

… a palette is specified as run-time parameter

Saves ~10% of compile time for me by skipping redundant palette check when using nfo output (grfcodec will validate the palettes later).

@andythenorth andythenorth force-pushed the conditional-palette-validation branch 3 times, most recently from 164f1dc to eac0413 Compare June 10, 2022 21:45
andythenorth added a commit to andythenorth/iron-horse that referenced this pull request Jun 10, 2022
… skip input file validation with nfo output, assuming that nmlc was compiled with OpenTTD/nml#254 applied
@andythenorth andythenorth force-pushed the conditional-palette-validation branch from eac0413 to b472ac6 Compare June 25, 2022 07:26
@andythenorth
Copy link
Contributor Author

Diff looks wrong currently with respect to black formatting.

@Eddi-z
Copy link
Contributor

Eddi-z commented Jun 25, 2022

isn't this logic backwards? if i give a palette as parameter, i'd expect that the palette gets checked, otherwise i don't care?

also, does grfcodec do the same kind of checks on the palette as nmlc?

@andythenorth
Copy link
Contributor Author

andythenorth commented Aug 13, 2022

isn't this logic backwards? if i give a palette as parameter, i'd expect that the palette gets checked, otherwise i don't care?

Fail safe.
Vanishingly few nml users will ever need or want to disable that check. Most will not understand it as an optional parameter (or find it).

EDIT: no you're right the logic does appear to be backwards in the current implementation. It's an unfortunate artefact of needing to know explicitly which palette to set for action 14, which is otherwise currently inferred during the palette check (the approach to inferring palette seems extremely odd to me, but eh, maybe I missed something).

also, does grfcodec do the same kind of checks on the palette as nmlc?

No. It checks for invalid white pixels and invalid colour profiles. It does not validate, e.g. animated pixels etc. Accepted risk.

@andythenorth andythenorth force-pushed the conditional-palette-validation branch from b472ac6 to a3ed233 Compare August 13, 2022 20:00
@andythenorth andythenorth force-pushed the conditional-palette-validation branch from a3ed233 to ed10be9 Compare August 27, 2022 10:39
… a palette is specified as run-time parameter
@andythenorth
Copy link
Contributor Author

Superceded by #322 - this route involves inferring that we want to skip validation if we're outputting .nfo, which is unwise.

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.

2 participants