-
Notifications
You must be signed in to change notification settings - Fork 130
tweak(drawable): Decouple material opacity of detected stealth models from render update #1963
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
base: main
Are you sure you want to change the base?
tweak(drawable): Decouple material opacity of detected stealth models from render update #1963
Conversation
|
This also fixes #1719 , they are the same issue |
xezon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am confused by the 2 commits. Perhaps open 2 Pulls with each proposal?
Overall, I do not understand why the m_secondMaterialPassOpacity evolution is made so complicated.
| // TheSuperHackers @tweak The opacity step is now decoupled from the render update. | ||
| // minOpacity = (X ^ (framerate / updatesPerSec)) -> e.g. [ 0.05 = X ^ (100 / 2) ] -> [ X = 0.941845 ] -> [ 0.941845 ^ 50 = 0.05 ] | ||
| constexpr const Real minOpacity = 0.05f; | ||
| constexpr const Real updatesPerSec = 2.0f; // (LOGICFRAMES_PER_MSEC_REAL / data->m_updateRate (15)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the value code commented? Edit: This needs to be fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another question: Does this still work correctly when pausing the game?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, the opacity keeps decreasing and at some point it gets below the minimum of 0.001 and so it gets set to 0. At that point the regular model is shown instead. This'll need fixing.
| if (theirDraw && !them->isKindOf(KINDOF_MINE)) | ||
| { | ||
| theirDraw->setSecondMaterialPassOpacity( 1.0f ); | ||
| constexpr const Real minOpacity = 0.05f; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this value should be part of drawable data? It now exists at 2 places and it needs to be in sync?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, having it in two places is unfortunate. I think making it a part of the drawable date should work.
The second commit is a less optimized but simpler implementation of the first commit, that's all.
I think we can forget about the first commit if we decide that calculating the scalar value on the fly is acceptable.
Right, I'll add an explanation. This was the original behavior assuming 30 logic frames and 30 render frames per second.
It's not overly complicated, but since the opacity decreases logarithmically, we need to calculate the right scalar value.
See point 3. I've increased it slightly to a more sensible number. |
xezon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I understand the logic now and it looks correct. We need to improve this change a bit.
| constexpr const Real minOpacity = 0.05f; | ||
| constexpr const Real resetThreshold = 2 * minOpacity; | ||
|
|
||
| // TheSuperHackers @tweak Reset opacity only below threshold to prevent models flickering from multiple detections or special powers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean multiple detectors will reduce the m_secondMaterialPassOpacity faster?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the opposite. Multiple detectors (and also increased logic frame rate) would set the opacity back to 1.0f faster / more often without this check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I understand. Would it perhaps be possible to couple the reset at exactly 15 frames (aka updateRate), instead of testing for reaching this min limit? Maybe build a logic frame counter for it?
Then we also do not need MinOpacity at 2 places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That'd mean the pulsating effect increases as the logic frame rate increases; e.g. at 60 logic frames the effect goes through 4 cycles per second instead of 2.
Does that make sense for things that are purely visual? I'm aware that some other visual effects currently behave this way (such as the clouds, water, trees, and the lights around the supply dock), but I'm not sure if that's desirable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right that makes no sense. We do not want it to be bound to plain logic frames. But you can scale the updateRate by LOGICFRAMES_PER_SECOND/BaseFps, which will be 15*2 at 60 logic fps. I think it would be interesting to know if there are issues with pausing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think StealthDetectorUpdate::update can simply set a boolean in the drawable that signals that the opacity may be reset back to 1.0, and then the draw code can handle that. I've just tested this and it appears to work well. It means that multiple stealth detectors and increased logic frame rate make no visual difference.
Performance wise, are you ok with calling pow on every call to Drawable::draw for every detected stealth unit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could store the scalar value in global data and update once per render frame, but I could use some suggestions which of the singletons would be most appropriate for that. That logic would also need access to the updates per second value and minimum opacity, so I'm not sure how to do it efficiently and cleanly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can also put static variables in any related class and use that.
I assume currently all objects blink independent of each other. If a static variable would control the evolution, then all objects would blink at the same time. Is that ok or desirable?
I think StealthDetectorUpdate::update can simply set a boolean in the drawable that signals that the opacity may be reset back to 1.0
Wouldn't the theirDraw->setSecondMaterialPassOpacity(1.0f) in the logic update already implicitly signal the reset?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can also put static variables in any related class and use that.
The idea is to put the scalar value inside Drawable as static inline variable, and create an update function that recalculates it on every frame and is called from somewhere high up in the engine loop.
If a static variable would control the evolution, then all objects would blink at the same time. Is that ok or desirable?
They would still be updated at different times, so they would only blink at the same time if they were detected by the same stealth detector (or at a similar 15 frame interval).
Wouldn't the
theirDraw->setSecondMaterialPassOpacity(1.0f)in the logic update already implicitly signal the reset?
That's not implicit but explicit as the logic code would set the value back to 1.0f. What I'm proposing is not setting the value directly, but setting a boolean, so that the render code can check that to set the value back when needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated commits to show new implementation. I got to figure out how C++98 handles static inline class member variables, though...
… from render update Scalar value is calculated ahead of rendering.
… from render update Scalar value is NOT calculated ahead of rendering.
8e09433 to
21dd3e3
Compare
546c2d9 to
45d821e
Compare
|
I'd suggest to make a small change to the blinking / pulsating behavior: lower the update rate to once per second and increase the minimum opacity to 0.1. It looks like this: stealth_detected_model_change.mp4 |
Fixes issue: #1953This PR makes two visual changes relating to the opacity step of detected stealth models.
There are 2 commits, with 2 slightly different implementations. The first was an attempt to make the code more optimized. It has a downside: if the render frame rate is fluctuating a lot, the opacity step can be slightly irregular sometimes but nothing crazy. The second calculates the opacity step scalar when rendering. This gives the best results but is more costly.Before
stealth_detected_model_flickering_before.mp4
stealth_detected_model_pulsating_before.mp4
After
stealth_detected_model_flickering_after.mp4
stealth_detected_model_pulsating_after.mp4
TODO