lethal-guitar
commented
2 years ago Nice one, thanks a lot for the contribution! I'll check it out as soon as I have time 🙂
Open Nutzzz opened 2 years ago
lethal-guitar
commented
2 years ago Nice one, thanks a lot for the contribution! I'll check it out as soon as I have time 🙂
Nutzzz
commented
2 years ago Looks like some of the builds want newer-style C++ casts.
After creating the pull, I had some thoughts:
lethal-guitar
commented
2 years ago
- Looks like some of the builds want newer-style C++ casts.
Yeah, I have pretty strict warning settings - either static_cast or constructing an instance of the target type, i.e. uint8_t(...), should do it. I was thinking though if maybe we should use base::round here instead of casting. The difference is probably too subtle to really be noticeable, but it feels more correct somehow.
- It's sometimes pretty weird what's considered foreground vs. background. See pic below, from an earlier test, where yellow = backdrop, cyan = background, and magenta = foreground. I thought about having one value with some kind of algorithm where foreground is brighter than background which is brighter than backdrop... except Apogee has already picked colors that effectively dimmed the backdrop (of the first level at least), so I just threw all 3 settings in there for testing... and then just left it that way for now.
Yeah, unfortunately the assignment to foreground or background doesn't always match what you'd expect - it's really just there to determine which tiles appear in front of actors and other sprites, and which ones are behind.
I guess what would be nice is for the tiles that have collision to always be considered "foreground"? I'd have to check if that's already the case or not.
If not, one thing we could do is to set the "foreground" bit for all tiles that have any of the "solid" bits set, e.g. in here:
- As far as naming the setting: First, maybe the setting should include the word "tiles" since actors aren't affected. Second, is it clear what a "backdrop" is? Maybe it should be "deep background"?
Maybe "parallax background"?
Third, maybe the setting should be called "gamma" instead of "dim" (otherwise the scale of 0.0 to 1.0 seems backwards, i.e., it defaults to 100% dimness rather than 100% gamma.
Right, I see what you mean. I'd suggest "brightness", since gamma has a specific meaning involving an exponential curve, which we don't use here.
I was also wondering where to place these settings in the options menu. There's already one accessibility setting in the "Graphics" tab, a checkbox for disabling screen flashing. Maybe it makes sense to add a new tab "Visuals/Accessibility", move that screen flash option in there, and then add the new background darkening options also there? What do you think?
Nutzzz
commented
2 years ago So I've incorporated all of your suggestions other than renderer::saveState() which wasn't working for me. I've also added sprite foreground/regular actor sprite brightness settings, and added a new class of background actor sprites; this is to resolve the issue discussed in #833 with the edges of the jet flames, water/lava falls, etc. I've also made the prisoner monsters optionally background so the bars can match.
lethal-guitar
commented
2 years ago @Nutzzz cool, that all sounds good! I'll check out your changes as soon as I get around to it. Btw, in case you haven't seen it yet, there's a script tools/format-all.sh which you can use to run clang-format locally. There's some additional info here.
Nutzzz
commented
2 years ago Btw, in case you haven't seen it yet, there's a script
tools/format-all.shwhich you can use to run clang-format locally. There's some additional info here.
Thanks for that. I figured there was something I was missing. Visual Studio recognized the .clang-format file, but obviously it wasn't quite doing the trick (at least the way it's currently configured for me).
lethal-guitar
commented
2 years ago Thanks for that. I figured there was something I was missing. Visual Studio recognized the .clang-format file, but obviously it wasn't quite doing the trick (at least the way it's currently configured for me).
Sure thing! Visual Studio's clang-format integration doesn't work for me either. They most likely have a different version of the formatter bundled. Unfortunately, with clang-format, the configuration file is not enough - the version also needs to match
lethal-guitar
commented
2 years ago Finally had a chance to play around with this - it's quite nice, definitely adds to the game!
I noticed that the tile color mods were only applied to the static parts of the map - dynamic parts like shootable walls, falling rocks etc. would always appear in the regular color. These are handled in a separate code path nowadays, this was probably still different at the time Calinou made the WIP branch. I pushed a commit to fix it.
Some more thoughts:
Finally, I wonder how to deal with things like ladders and climbing pipes. These seem to always use background tiles, but it would be nice if they did still use the foreground brightness, since they can be interacted with. Making them part of the background could make it a bit harder to notice. I don't have a good idea at the moment how to implement that though, will think about it a bit.
lethal-guitar
commented
2 years ago These stairs are another edge case:
They use background tiles, but should maybe use the foreground brightness since they have collision.
Nutzzz
commented
2 years ago Finally had a chance to play around with this - it's quite nice, definitely adds to the game!
Yes, I find the game much more enjoyable with this enabled.
dynamic parts like shootable walls, falling rocks etc. would always appear in the regular color.
Ah, that seemed to be a feature initially, but you're right that if we're going to continue to let the regular/foreground sprites be configurable, then those ought to be added also.
- Maybe the background sprite brightness should always use the background tile brightness value to simplify the configuration for the user - i.e., we don't offer a separate slider, but just use the background tile value.
I started it that way, but as before I made it maximally configurable for my testing, figuring it would be pared back at some point. I'd probably combine regular and foreground sprite brightness as well, if not removing it as an option entirely.
- Maybe the minimum brightness should be capped to a value > 0? Making a layer completely dark doesn't seem useful to me, but let me know if you disagree.
If this is really an accessibility feature, then backdrop should definitely be settable to zero. As far as the background tiles, then it really depends on whether we can come up with an answer to the ladder/pipe/stair thing you mention. It at least deserves a warning at this point.
- If we keep the prisoner option, we should either prevent the option from being toggleable while in-game, or make it apply immediately when changing it like the other options. Currently, it can be toggled while in-game but this doesn't have any effect, since the actors aren't recreated when toggling the option.
I've only included it as an option because of the bar color mismatch. Obviously, the aggressive ones can't be too dark, so if we removed the option they'd have to stay regular sprites. I thought about setting just the passive ones as background but that ruins the "is-it-the-one-or-the-other" thing. I suppose the sprite could be modded separately to limit the severity of the issue.
Finally, I wonder how to deal with things like ladders and climbing pipes [EDIT: and stairs]. These seem to always use background tiles, but it would be nice if they did still use the foreground brightness, since they can be interacted with. Making them part of the background could make it a bit harder to notice. I don't have a good idea at the moment how to implement that though, will think about it a bit.
:-) I had the same thought and the same lack of ideas...
lethal-guitar
commented
2 years ago Ah, that seemed to be a feature initially
Yeah, I see what you mean - playing with it a bit more, I found various cases where things like shootable walls now use the background brightness but really should be using the foreground one. What complicates this is that not only the dynamic parts themselves go through the separate code path, it also affects tiles below falling map geometry (because these get erased as the dynamic piece moves across). Where I initially noticed the discrepancy was here (this screenshot is from before my extra commit):
Because the two rocks can fall down and destroy parts of the wooden beam while doing so, those parts of the wooden beam are rendered using the dynamic geometry path, making them appear brighter than the rest of the beam. All the tiles making up that beam are actually part of the background.
Here I'm also not quite sure how to solve it. I guess we'd definitely want to apply the "foreground" brightness to any dynamic geometry pieces themselves, i.e. the falling rocks in this example - but then use the appropriate brightness, i.e. either background or foreground, for tiles that are in the path of a falling rock.
I started it that way, but as before I made it maximally configurable for my testing, figuring it would be pared back at some point. I'd probably combine regular and foreground sprite brightness as well, if not removing it as an option entirely.
Ah I see, yeah that makes sense.
If this is really an accessibility feature, then backdrop should definitely be settable to zero. As far as the background tiles, then it really depends on whether we can come up with an answer to the ladder/pipe/stair thing you mention. It at least deserves a warning at this point.
Right, makes sense. Just a warning could be totally fine I think.
Obviously, the aggressive ones can't be too dark, so if we removed the option they'd have to stay regular sprites. I thought about setting just the passive ones as background but that ruins the "is-it-the-one-or-the-other" thing.
Totally, that was my thought process as well. As you mentioned, maybe adjusting those specific sprites is the best way to go here. That way, you could even have them both be generally background, but the aggressive prisoner's hand could then become foreground during the grab animation.
lethal-guitar
commented
2 years ago @Nutzzz just pushed another commit, which makes it so that the prisoner's grabbing hand will always be regular/foreground. It's still not perfect, since this also increases the brightness of parts of the bar and the monster's right eye, but maybe good enough for a first iteration? Let me know what you think.
https://user-images.githubusercontent.com/1821027/195987868-b5127cfa-7bda-4e87-ab59-e0da7c23a8b0.mov
Regarding the ladders, pipes, stairs etc., I've also thought about this a bit and I think pipes and stairs might not be too hard to do, but ladders I don't think we can make work without providing some extra metadata about the tilesets. The issue is that the "active" part of a ladder is often not the same as the visual representation, for example here (the blue boxes indicate tiles with the "ladder" flag set):
So while we could use the tile's "ladder" flag to make those tiles use the foreground brightness, the rest of the ladder would still appear darker. Since the visual width of ladders isn't always the same, we unfortunately have to manually specify this for each tileset.
That would mean however that it only works for the stock assets coming with the game, a mod with a custom tileset for example wouldn't have this metadata. We could provide a mechanism for modders to specify this for their tilesets, of course, but this all starts to get a bit complicated.
All of this makes me think that maybe for now the best is to keep ladders as "background" tiles - it's not perfect, but there's still a lot of value in this change, and maybe we can find a good solution for ladders later on?
lethal-guitar
commented
2 years ago @Nutzzz Ah, I should have mentioned, I rebased and force pushed to your branch. If you want to get my latest state, I recommend doing git fetch; git reset --hard origin/<branch_name> instead of git pull since the latter doesn't really know how to deal with rebased branches. I've pushed my local version of the branch here: https://github.com/lethal-guitar/RigelEngine/tree/add-darken-background-option
So if you want to get a clean version of my latest state, you could hard reset yours to that branch and then force push again.
Nutzzz
commented
2 years ago So if you brightened the ladder it would look something like this? If so, I feel like it has merit even if it's not perfect...
(Note this background brightness is 0.100 where it's pretty hard to see the ladder. I was trying to decide where the warning needs to be made; maybe 0.150?)
lethal-guitar
commented
2 years ago So if you brightened the ladder it would look something like this?
Yeah, exactly. To quickly hack it and try it out, you could first extend the condition here:
to also include map.attributeDict().attributes(tileIndex).isLadder(). Could also add isClimbable and isFlammable.
Then, in the loop where the animated tiles are drawn:
You could do the same attribute check (using mpTileAttributes instead of map.attributeDict), and if it's ladder/climbable/etc., you would temporarily set the color mod back to default for drawing that tile. That should then make all ladder/climbable/etc. tiles appear at full brightness.
lethal-guitar
commented
2 years ago Just realized, an even simpler hack would be to change
so that it also returns true if any of the ladder, climbable or flammable bits are set.
Edit: Nevermind, that would make these tiles appear in front of the player and other sprites.
Based on Calinou's request and WIP branch, this improves the game's accessibility by adding an option that emphasizes dynamic objects in the game. ( Issue #833 )