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

Allow map memory drawing mode selection through options #32713

Merged

Conversation

ZhilkinSerg
Copy link
Contributor

Summary

SUMMARY: Interface "Allow map memory drawing mode selection through options"

Purpose of change

Allow map memory drawing mode selection through options

Describe the solution

Exposed map memory mode selection to Graphics tab of Options menu. Defaults to "Modern (Sepia)" and also allows selection of "Classic (Gray)" value. Changing this options requires restart or tileset reload.

Describe alternatives you've considered

As outlined in #32669 we want to make this option configurable (either in a tileset or via a tileset mod). Current PR is just a band-aid for those retrogrades who don't like sepia.

Additional context

image

@ZhilkinSerg ZhilkinSerg added Info / User Interface Game - player communication, menus, etc. [JSON] Changes (can be) made in JSON SDL: Tiles / Sound Tiles visual interface and sounds. [C++] Changes (can be) made in C++. Previously named `Code` labels Jul 29, 2019
@MeltedClowns
Copy link

This is exactly what I want the second the update came out! (Glad I'm not alone)

@ZhilkinSerg ZhilkinSerg force-pushed the sdl-map-memory-shading-options branch from 7261932 to 13922c4 Compare July 29, 2019 07:14
@ZhilkinSerg ZhilkinSerg marked this pull request as ready for review July 29, 2019 07:28
@ZhilkinSerg
Copy link
Contributor Author

Jenkins rebuild

@ProfoundDarkness
Copy link
Contributor

Count me among the anxious for this as well.

@ZhilkinSerg
Copy link
Contributor Author

Jenkins rebuild.

@kevingranade
Copy link
Member

This pull request has been mentioned on Cataclysm: Dark Days Ahead. There might be relevant details there:

https://discourse.cataclysmdda.org/t/going-back-to-original-grey-instead-of-sepia-for-memory/20940/2

@kasanryukin
Copy link
Contributor

Sepia looks horrible on my monitor, and I'm willing to bet it does for a lot of folks too. I'm glad to see a quick fix to have a choice between classic grey and modern sepia is ready to be merged.

@kevingranade
Copy link
Member

This pull request has been mentioned on Cataclysm: Dark Days Ahead. There might be relevant details there:

https://discourse.cataclysmdda.org/t/how-to-change-sepia-brown-to-old-gray-colors-tileset-only-compile-only/20948/2

@esotericist
Copy link
Contributor

I think calling this "gray" or "grayscale" is misleading; we use grayscale for low-light vision, and it's genuine grayscale, while the "classic" map memory was full-color but reduced in intensity (and isn't actually having its saturation reduced, so it's no more "gray" than the default tiles, just darker).

Maybe use different terminology? "Muted" is the best I can come up with offhand, "darkened" is another option.

@chphilli
Copy link

chphilli commented Aug 2, 2019

I'm curious about the status of this PR. I suspect that there's a performance issue with the sepia feature at least on my machine when moving around causes bunch of tiles to need to be drawn in sepia -- and based on my reading of the code this update will allow me to test the paths.

I don't quite have my setup in place to build it myself and test it, so I'm hoping that this PR goes through and I can test that way.

Thanks!

@esotericist
Copy link
Contributor

I'm curious about the status of this PR. I suspect that there's a performance issue with the sepia feature at least on my machine when moving around causes bunch of tiles to need to be drawn in sepia -- and based on my reading of the code this update will allow me to test the paths.

I don't quite have my setup in place to build it myself and test it, so I'm hoping that this PR goes through and I can test that way.

Thanks!

The sepia change only altered how the memory map tile assets are generated. This only occurs when the tileset is reloaded.

It should not be possible for the sepia change to impact runtime performance.

@ZhilkinSerg
Copy link
Contributor Author

Status of the PR is open.

There are no performance in-game issues either with sepia or without it. All tile variants are created during tileset loading - usually once per game session.

src/options.cpp Outdated Show resolved Hide resolved
src/sdl_utils.cpp Outdated Show resolved Hide resolved
src/sdl_utils.h Outdated Show resolved Hide resolved
src/options.cpp Outdated
add( "MEMORY_MAP_MODE", "graphics", translate_marker( "Memory map drawing mode" ),
translate_marker( "Specified the mode in which the memory map is drawn. Requires restart." ), {
{ "color_pixel_darken", translate_marker( "Classic (Darkened)" ) },
{ "color_pixel_sepia", translate_marker( "Modern (Sepia)" ) }
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think both "Classic" and "Modern" names are really needed here. What if someone adds another drawing mode? Call it "Super-Modern" or "Neo-Modern"? Simple "Darkened" and "Sepia" is enough.

src/options.cpp Outdated Show resolved Hide resolved
src/options.cpp Outdated Show resolved Hide resolved
@esotericist
Copy link
Contributor

Well, I'm happy with this.

@esotericist esotericist merged commit d4a8eb0 into CleverRaven:master Aug 6, 2019
misterprimus pushed a commit to misterprimus/Cataclysm-DDA that referenced this pull request Sep 21, 2019
…32713)

* Allow map memory drawing mode selection through options

* Apply suggestions from code review

* Apply suggestions from code review
@ZhilkinSerg ZhilkinSerg deleted the sdl-map-memory-shading-options branch September 22, 2019 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[C++] Changes (can be) made in C++. Previously named `Code` Info / User Interface Game - player communication, menus, etc. [JSON] Changes (can be) made in JSON SDL: Tiles / Sound Tiles visual interface and sounds.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants