Repository navigation
Conversation
|
I haven't reviewed the code in detail yet, but I have been testing the feature.
Regarding the commits, STK's repo is set to squash-and-merge (this avoids the noise from merge commits, and for most PRs it helps to make the history cleaner). I think this may be a case where it's actually cleaner for code history to have both separated. So, once testing is complete and the code has been polished, it would make sense to have the video settings code in a separate PR. |
|
Just tested and I love the effect. Imo it's in a great spot of helping with the feel of speed while not being distracting. I haven't tried around with any parameter yet, I might do that at some point, but the effect looks great as is, so there are not really any modifications needed. I agree with Alayan that the activation happens a bit too easy. |
Alayan-stk-2
left a comment
There was a problem hiding this comment.
Here are all the things I noticed while checking the code.
The issue of the look of the effect in ultra-wide I mentioned previously also applies.
Also, speed streaks don't show up with ghost karts (probably because you can't properly probe their speed)
| // ------------------------------------------------------------------------ | ||
| /** Returns the speed amount above the kart's own unboosted max speed. | ||
| */ | ||
| float AbstractKart::getCurrentAdditionalSpeed() const |
There was a problem hiding this comment.
Don't put any actual logic in AbstractKart.
AbstractKart is only inherited by Kart, so in the Evolution branch it has been fully removed as redundant and cumbersome.
In the 1.x codebase, you still need to get functions there so callers with an abstract kart can get things in and out, but actual logic here will make the future merging process into the Evolution branch more difficult.
| { | ||
| m_skidmarks->reset(); | ||
| } | ||
| if (m_speedstreaks){ |
There was a problem hiding this comment.
Braces at the end of an if line don't follow STK's style conventions
| intensity = speed_streaks->getBoostIntensity(); | ||
|
|
||
| auto direction = SpeedStreaks::Direction::FORWARD; | ||
| if(cam->getMode() == Camera::CM_REVERSE) |
There was a problem hiding this comment.
The fact that you have the speedlines move in the right direction in reverse camera is a nice touch.
You can probably handle the assignment of direction with a simple ternary, it's cleaner.
| { | ||
| World* world = World::getWorld(); | ||
| Camera* cam = Camera::getActiveCamera(); | ||
| const Kart* kart = cam ? dynamic_cast<const Kart*>(cam->getKart()) : nullptr; |
There was a problem hiding this comment.
This test is absolutely insufficient to know if the camera should actually have speed streaks displayed.
For example, the free camera (ctrl+F2) will still have speed streaks displayed. So does the top view camera. So do the various side of view cameras. Most of these cameras are logically tied to a kart, too.
Some kind of white list of camera modes where speed streaks make sense is probably the way to go.
| } | ||
| if (m_speedstreaks){ | ||
| m_speedstreaks->reset(); | ||
| }; |
There was a problem hiding this comment.
This ; does nothing and should be removed.
| const std::string noiseTexturePath = file_manager->getAsset( | ||
| FileManager::AssetType::TEXTURE, | ||
| "stk_noise_simplex.png" | ||
| ); |
There was a problem hiding this comment.
The closing parenthesis on its own line isn't really something done in STK's codebase.
| float calcBoostIntensity(float lower_bound_ratio = 0.0f, float upper_bound_ratio = 1.0f) const; | ||
|
|
||
| // Reference to the kart to which these speed streak parameters belong. | ||
| const AbstractKart& m_kart; |
There was a problem hiding this comment.
In postprocessing, you actually already retrieve a Kart object rather than an AbstractKart object. So we should use Kart here too for the reason explained in my other comment (no AbstractKart class in the Evolution branch).
|
|
||
| #include "graphics/speed_streaks.hpp" | ||
| #include "karts/abstract_kart.hpp" | ||
| #include <irrMath.h> |
There was a problem hiding this comment.
A blank line is typically left in STK's codebase between the local STK includes and includes from libraries.
|
|
||
| // Speed excess (as a fraction of the generic engine max speed) at which the | ||
| // streak intensity starts rising: 0.0 = the kart's own unboosted top speed. | ||
| constexpr static float s_boost_intensity_lower_ratio = 0.0f; |
There was a problem hiding this comment.
Before I saw this value in code, I thought the threshold was higher, because the effect at very low intensity is barely noticeable. This isn't a bad thing though, it means it fades in more naturally.
However, the speed streaks are a bit too easy to activate as mentioned in the previous review, so this should be increased. Based on my tests, I'd say 0.2f should be fine.
| constexpr static float s_boost_intensity_lower_ratio = 0.0f; | ||
| // Speed excess (as a fraction of the generic engine max speed) at which the | ||
| // streak intensity reaches 1.0f. | ||
| constexpr static float s_boost_intensity_upper_ratio = 0.75f; |
There was a problem hiding this comment.
I think this could be increased slightly, let's say 0.8f or 0.85f.
If you accumulate speed boosts, more than 0.75 is actually not that difficult to reach.
A related issue is that the effect looks very weak at tall ratios close to 1:1. |
|
Hey both thanks for the review. I had a bit of a busy one this week, but I should be able to address the feedback tomorrow. |
|
The "Motion Blur" effect that's actually just a blur at the edge of the screen when activating a zipper should probably be either merged into this effect or removed entirely. (An actual motion blur would be nice to have available in STK, but that's a different issue.) |
|
@qwertychouskie I agree I think the motion blur could do with another pass. I was actually thinking after this PR merges I'm wanting to experiment adding proper motion blur to STK. We basically need to add an extra rendertarget to handle motion vectors and then change the post processing shader that's currently used for the radial motion blur and change it to use the motion vectors to handle the blur. Motion vectors are generally useful and can be used for a whole host of effects as well. Regardless we should start a separate thread on Motion Blur imo. |
This reverts commit c3a63e6. Will be recommited in a separate PR for clarify.

Why
As discussed in the forums, I've been working on adding a speed streaks post processing effect to the game. The main purpose is to add to the player's experience of speed in the game.
Changes
The PR is split in two commits, in the first commit the notable things I've implemented:
data/shaders/speed_streaks.frag, I've added extensive comments to explain how the shader works, but in a nutshell we convert UV coords into polar coordinates and then sample a single simplex noise (twice by using different UV scaling) and then layer them to extract noise peaks (a.k.a wisps). There are quite a few tweakable parameters, like density, softness, amount etc. that can be changed to fit the art direction. (I think I've done a pretty good job :) , but feel free to make changes if needed).src/graphics/post_processing.cpppassing in the main uniforms that control the shaderu_intensity(0.0f - 1.0f value that controls the strength of the effect) andu_radial_scroll_offset(scrolling UV coordinates). As well as passing in a newly added seamless simplex noise texture (generated using FastNoise2 library) - to be added to the SVN repository. (See textures.zip)src/graphics/speed_streaks.hppandsrc/graphics/speed_streaks.cppclass to neatly organize and store the dynamic intensity value and the UV offset for individual instances, as well as some static parameters that can be tweaked. This, like the other effects that are related to the player, is stored on the Kart. Ensuring it works for swapping cameras, split-screen, online etc. setups.getCurrentAdditionalSpeedandgetCurrentAdditionalSpeedRatio) tosrc/karts/abstract_kart.cpp, I believe they have general utility in the codebase, but specifically to help calculating the boost intensity of the effect.As for the second commit, as requested I've added a toggle to the video settings so that we can turn the effect on and off. I've followed suit of the other effects and added the relevant bits, like adding the CLI option and adding it to the in game benchmark (profiler). I've not yet added it to be automatically included in any of the graphics presets, to be discussed and added as a separate PR.
Agreement