-
Notifications
You must be signed in to change notification settings - Fork 282
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
Rework infoLOS shader. Use a maximum intel based approach instead of additive. #1931
base: master
Are you sure you want to change the base?
Conversation
This probably fixes #1817 |
Have you perhaps seen my reimagings in df06d63 ? |
@icexuick have a looksie here |
I see that the old shader code is duplicated in df06d63 ; I don't understand why. |
It's been many months since I made this PR. I play in a lot of noob lobbies and I get the impression that people don't understand areas that are and aren't covered by radar and I think this would help. What is holding this up? |
One of your major premises is false, radar is not obsoleted by sight because it can see cloak where sight cannot. |
@sprunk technically true. As you correctly pointed out, LOS without radar coverage and LOS with radar coverage are different in some cases, but notably, the game currently doesn't distinguish the two visually; it's never been a problem before, so I hope it wont be considered a problem now. Also, there is a limited amount of contrast available, and using some of that contrast on the edge case of cloaked units isn't worth it IMO. |
My only 2 'things' to talk about are:
This PR does do what it says, it simplifies the LOS layers and also cleans up some strange/goofy effects.
Things I'd like to see improved:
|
This is something we need to take care of ourselves, it is not the player's responsibility to adjust the shaders for each map. If we need to create a per-map config, then that's what we'll need to do. I have never been a fan of the diagonal lines, but that is outside the scope of this PR. |
I'll reply to @icexuick's comment in the same order he made his points:
Regarding cloaked units. I've maximized the fog of war opacity, but I cannot see any indication of radar coverage inside LOS. For example, here's some ticks on Forge, without radar (there isn't a single radar anywhere on the map). I think this might be a case where you have edited your widgets to achieve an effect that is not normally possible in the game? See the screens below, these are from the default game. Screencast.from.2024-02-25.15-09-32.webmAs I mentioned above, the game doesn't normally show radar coverage within LOS currently, and as far as I'm aware nobody has complained about it. We need to decide whether that's a concern that will be addressed in this PR, and if so, I don't know if I'll be able to fix it.
This also is beyond my current knowledge to solve. The LOS shader adjusts colors based only on map coordinates. It does not know the position of the players camera, it does not know the players zoom level, etc. Also, the LOS shader doesn't know the color of the map. We can easily adjust the width of the diagonal lines, but I don't know how to to make them dependent on player zoom level or map color.
This is true but was was not caused by my change. I remember talking with @Beherith about this on Discord, but haven't been able to find the conversation. What I remember is that the texture containing the LOS information is lower resolution that the actual map (for performance reasons), and a lot of blurring happens when applying that lower resolution texture to the map. This is why the edges are not sharp, and I don't know how to make them sharp. This has always been a problem but the fog of war effects have been so subtle nobody noticed. Is this something we need to fix in this PR? See this screencast from the default game (without my changes): Screencast.from.2024-02-25.15-37-11.webm
Agreed, currently there are no adjustable settings, and this change breaks the opacity slider because I couldn't get the opacity calculation in lua to work well with the shader. See relevant code comment: https://github.com/beyond-all-reason/Beyond-All-Reason/pull/1931/files#diff-12d6cb52e62d8431817023ddaa609f3de5f8b416917474f4d4ea5f7803f71ca5L61
As mentioned, the shader does not have access to the color of the map. If it did, we could adjust the shading to handle brighter / darker maps, but we cannot. I don't know how to fix this. Is this something that needs to be fixed in this PR? Notice my screenshot of Forge from above, it comes from the default game. The brightness seems about the same to me. |
Here's some screenshots from Forge. The brightness is not that different IMO. With this PR:Without this PR (this is the default game, with 100% fog of war opacity):This last screenshot shows bug #1817 Note, the custom shader I put on Discord is not the same as this PR. The custom shader on Discord makes more extreme changes, and this PR makes more subtle changes. So be careful if you are judging this PR based on the Discord download. |
Not sure why my Forge game shots are much darker. Regarding your other feedback @DevJac:
|
Something is weird with the green radar ring from the commander. The fog of war is a more accurate indicator of radar range than the green ring. Take a look at these screenshots. The first has The green ring is wrong. It might be an air LOS indicator or something? |
Scratch that, I don't know what I was thinking. I'm using an Nvidia GPU on Linux. |
I can confirm that the radar on the dotted line is indeed too large, and the LOS shader seems to be on point. |
I think the big problem we have with differentiating areas from one another is that we use a gradient where they meet, instead of a hard border. That problem isn't specific to this pull request, but I think solving that will go a long way to solving this issue, no matter how we style the different areas. |
@WatchTheFort: I believe the radar coverage (and jammer, etc) is stored in a texture that is lower resolution than the map. That is why the boarder is fuzzy, because we don't have the information needed to draw a perfectly sharp boarder. There would be a performance cost if we store that data in a higher resolution texture. |
@icexuick What map is that? I want to compare the radar / los contrast with the base game. As before, I suspect we will find that the base game already has a difficult to see transition between radar / los, but it will be good to check. Actually, did we ever resolve this? I showed screenshots (from my computer) that showed little difference between this shader and the default game, but it sounds like you were seeing something else? Are we seeing the same things? I'm open to talking on Discord and sharing screens or whatever. |
Well i think hardlines will not look that 'pretty'. |
I don't think the hard lines look that bad, but I don't know how to change them. Maybe half way between the current smoothing and what we currently have? |
Yes, I might be wise to jump in a VC / screenshare to discuss some of the topics. Is there an option to 'sharpen' the edge with your new shader? |
PS. please make a thread on Discord so we can channel all feedback there. |
Pinged you on Discord. But I want to reply to your feedback about that map and the low los / radar contrast. I'm calling the 3 intel levels "los" / "radar" / "fow". "fow" meaning no radar coverage. (There are also things like los with radar and los without radar, but we've talked about that previously in this PR. There's only so many information channels available.) That's an unavoidable trade-off here. We want los to remain roughly the same brightness, and fow to remain roughly the same brightness, and radar to be somewhere in between. Currently radar has the same brightness as fow. That's my complaint and the reason I made this shader, there is no contrast between radar and fow areas. To fix this I moved the radar brightness to be more in the middle between the two extremes. The brightness follows a roughly linear progression from los -> radar -> fow. But, moving the radar brightness adds to one contrast while taking away from the other. In this case, the contrast between los and radar is reduced. Let me illustrate it with some ascii art:
Yes, the contrast (color distance) between los and radar has decreased, but the contrast between radar and fow increased. I think I've beat this point to death now. Of the two, I prefer having the radar contrast closer to los, because los is usually quite predictable, and there are also los lines that can be turned on in the settings (I have them on). (I've said similar on Discord if you want to discuss there: https://discord.com/channels/549281623154229250/1132003042007449681/1254549192961953903 I wanted to keep major points of discussion here in GitHub as well.) |
The fact that there are LOS rings at all is completely bonkers. There is zero reason they should ever exist. |
Units can be in a position where their LoS range is not evident regardless of how you improve LoS coverage visibility on the map:
|
@icexuick What is the status of this? Is more work on it needed? Are we planning to merge it to the game? |
Thats a good question - this has slipped through my fingers. |
I tried running this, but I think it's not working anymore. When I hit ; to disable/change the with/without radar i looks roughly like in the screenshots. I do like no radar + no vision is more clear. |
Can we please get rid of |
It's been a long time since I played BAR, and has been over a year since I looked at the code in this PR. I'm not interesting in working on this or contributing to BAR at the moment. If you want to close this PR, I understand. |
Overview
This PR simplifies the
infoLOS
shader and renders the fog of war based upon the maximum level of intel instead of the purely addition based approach of the old shader (I'll explain why this additive approach was a problem below.)Also, the old shader was doing some really weird things. I believe the first evidence of this is how much simpler this shader is than the old one while still achieving very similar results. Another small example is the line
mix(diagLines2(texCoord), diagLines1(texCoord), heightStatus)
from the old shader, in whichheightStatus
could be -3 (negative three); that's quite unusual. The old shader also had unreachable code which I removed.My goal in doing this was to make the areas that are not covered by radar more visible. To achieve this, I increased the width of the so-called "terra-incognita" lines. I also (ab)used the radarColor1 to allow the darkness of the terra-incognita lines to be adjusted outside of the shader. I will personally be adjusting this in a widget, but tried to leave the default colors and shader subtle like the existing one.
Why a maximum intel approach is better than an additive approach.
A problem with the old shader is that every form of intel adds to the brightness. You want radar to add some brightness to the map to indicate coverage, and, of course, you also want direct line-of-sight to add brightness. The problem then is that radar coverage makes the areas with direct LOS even brighter. This is goofy.
To illustrate the problem, imaging you have several grunts spread loosely around the screen. They have direct LOS over their area and so that area is fairly bright; there is no fog of war around your grunts. Now imagine someone builds a T3 radar half-way across the map, and now the area around your grunts becomes even brighter. That's weird. A radar half-way across the map adds no intel to the area with your grunts, why should it become brighter?
This is how the additive approach must be. If the radar contributes significant brightness, and direct LOS contributes significant brightness, then together they are too bright.
This shader uses maximum instead of addition. So someone building a T3 radar across the map will brighten the appropriate dark areas, but it will not brighten the areas where you already have direct LOS. This way both radar and direct LOS can contribute a significant amount of brightness without adding to become too bright.
Screenshots
Here's a screenshot comparing the old and new shader. The first screenshot shows the old shader.
It's worth noting that the first screenshot demonstrates a bug with the old shader. If you look closely at the water covered by radar, you can see that there are still subtle terra-incognita lines that were not fully removed. I think this is because the old shader was returning values greater than 1.0 for
gl_FragColor
. This bug does not occur with the new proposed shader.