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

Feature: allow limiting zoom level of NewGRF-provided sprites #8604

Merged
merged 1 commit into from Mar 13, 2021

Conversation

mattkimber
Copy link
Contributor

@mattkimber mattkimber commented Jan 23, 2021

Motivation / Problem

Some players find the combination of sprites from sets with and without 2x/4x alternative sprites visually jarring, to the point of unplayability in some cases. Although display of mismatching sprite resolutions can be prevented by limiting the overall maximum zoom, this can bring its own challenges when playing on small or high resolution screens where the extra zoom levels are necessary for fine placement of infrastructure.

This PR adds a setting to globally limit the maximum resolution of sprites in game, meaning extra zoom levels will be drawn using the in-game scaler rather than drawing higher resolution alternative sprites. It means players can choose to have greater visual consistency when mixing NewGRFs with/without high resolution sprites, without losing the ability to zoom in to 4x.

A secondary use case is for checking alignments and sprite details of 1x zoom levels in extra zoom sets; while this can be done by taking screenshots and expanding them in a paint program or changing the screen scaling and/or resolution settings, it would be convenient to temporarily zoom in while still seeing the normal zoom sprites.

Description

The GRF spriteloader already has a setting for checking zoom levels declared in a NewGRF are valid for the current version of the game. This can be extended to check against a configurable setting, so only the relevant zoom levels are loaded to the sprite cache.

As a version 2 NewGRF is the only vector by which an extra-zoom sprite could enter the game (even for base sets, I've tested this against zBase), this is sufficient to limit sprite zoom to the user's chosen zoom level, and allow them to choose whether to see extra zoom graphics or limit all sprites to the same resolution across multiple sets.

Limitations

If the user changes the setting in-game, sprites will not change until the window region becomes dirty. (This could feasibly be fixed by force-marking the entire window dirty when the setting is changed, I chose not to initially as players will typically alter the setting rarely, and dirtying the entire window may have other side-effects)

Changing this setting marks the entire window dirty, this is needed to avoid a mixture of different resolution sprites staying on screen until their area of the screen becomes dirty.

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

src/lang/english.txt Outdated Show resolved Hide resolved
src/settings_type.h Outdated Show resolved Hide resolved
src/spriteloader/grf.cpp Outdated Show resolved Hide resolved
@Eddi-z
Copy link
Contributor

Eddi-z commented Jan 25, 2021

Motivation / Problem

Some players find the combination of sprites from sets with and without 2x/4x alternative sprites visually jarring, to the point of unplayability in some cases. Although display of mismatching zoom levels can be prevented by limiting the overall maximum zoom, this can bring its own challenges when playing on small or high resolution screens where the extra zoom levels are necessary for fine placement of infrastructure.

I think this case could be handled with a warning, when maximum zoom level is increased, and not all base/newgrfs provide sufficient zoom level graphics (which, to my understanding, would be almost all the time), and a general UI scaling factor

A secondary use case is for checking alignments and sprite details of 1x zoom levels in extra zoom sets; while this can be done by taking screenshots and expanding them in a paint program or changing the screen scaling and/or resolution settings, it would be convenient to temporarily zoom in while still seeing the normal zoom sprites.

this use case should imho be guarded by "newgrf developer tools" being enabled, rather than an exposed setting which most people wouldn't really understand.

@mattkimber
Copy link
Contributor Author

I think this case could be handled with a warning, when maximum zoom level is increased, and not all base/newgrfs provide sufficient zoom level graphics (which, to my understanding, would be almost all the time), and a general UI scaling factor

I'd prefer not to have a warning and keep this as just a setting which players can change if they desire. Reasoning:

  • Plenty of players have no problem with mismatching sprites, sometimes even when the styles are completely different. It would be annoying to get a warning for doing something that has no adverse effects for many.
  • There is a small but vocal set of players who do dislike this effect. It feels reasonable to give them a setting to adjust it to allow them to use 2x newgrfs. (I'm not sure how large the set is or if complaining about zoom mismatch is "something to say for upvotes", although I had 2 requests so far for ways to limit Timberwolf's Road Vehicles to 1x)

this use case should imho be guarded by "newgrf developer tools" being enabled, rather than an exposed setting which most people wouldn't really understand.

I think the newgrf developer case is very much a mild secondary benefit, although there is definitely justification for this setting being in the "advanced" options to avoid too many spurious bug reports of, "why is my game low res even after I downloaded zBase?"

@2TallTyler
Copy link
Member

The default maximum zoom level is 4x, so that proposed warning would be generated frequently even without NewGRFs.

I think there may be some misunderstanding of the use case. This isn't fixing a problem, rather I see it as an enhancement to allow players to force extra-zoom NewGRFs to match the resolution of regular-zoom base sets and NewGRFs.

As one of the players who doesn't use regular-zoom and extra-zoom NewGRFs together (but also doesn't complain about said sets existing 🙂), I would eagerly use this feature.

@nielsmh
Copy link
Contributor

nielsmh commented Jan 25, 2021

I think there are some problems with the wording used in both this PR/comments, and possibly the menu text.
When I first saw this I thought it was about preventing the player from zooming in or something like that.
Then I read the new strings for the setting, and thought it would make some sprites not draw at all depending on zoom level (leaving holes in the landscape, which would be weird.)

Instead of talking about "zoom level" of sprites, perhaps call them "high resolution sprites" instead?
"Highest resolution sprites to use: 1x/2x/4x"
"Limit the maximum resolution to use for sprites. Limiting sprite resolution will avoid using high resolution graphics even when available. This can help keep the look unified when using a mix of GRF files with and without high resolution graphics."

@mattkimber mattkimber force-pushed the sprite-zoom-setting branch 3 times, most recently from 8d6a79c to 2be2340 Compare January 25, 2021 23:56
@EratoNysiad
Copy link
Contributor

I think this is a great addition, but I would like to ask: on servers, is every player able to set this setting for themselves?

@2TallTyler
Copy link
Member

I think this is a great addition, but I would like to ask: on servers, is every player able to set this setting for themselves?

Yes, this is a client-only setting. (You can tell because the setting definition in settings.ini includes flags = SLF_NOT_IN_SAVE | SLF_NO_NETWORK_SYNC)

@TrueBrain TrueBrain added this to the 1.11.0 milestone Mar 10, 2021
Copy link
Member

@TrueBrain TrueBrain left a comment

Choose a reason for hiding this comment

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

Almost done! Let's get this in for 1.11 :)

src/settings_type.h Outdated Show resolved Hide resolved
src/spriteloader/grf.cpp Outdated Show resolved Hide resolved
@mattkimber mattkimber force-pushed the sprite-zoom-setting branch 2 times, most recently from ef89127 to 4da5d9b Compare March 11, 2021 19:33
src/spriteloader/grf.cpp Outdated Show resolved Hide resolved
Copy link
Member

@TrueBrain TrueBrain left a comment

Choose a reason for hiding this comment

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

As there are no further comments :)

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.

None yet

8 participants