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

Improve visual feedback when using the motion vectors debug view option #80723

Merged
merged 1 commit into from
Aug 29, 2023

Conversation

DarioSamo
Copy link
Contributor

Replaces the current method of showing the raw values of the motion vectors buffer to display a grid of lines instead with a new shader. This should help make debugging motion vectors much easier.

This currently replaces the current method of viewing motion vectors. It should be decided whether to keep the old one as an option or not. Personally I don't see much use of it as if I want the raw values, I'd much rather inspect the texture on RenderDoc than look at RGB values that can't properly display the low range they usually have.

mvs.mp4

Side note: @clayjohn and I couldn't agree on what direction the vectors should point, so we'll let the people decide. :)

@darksylinc
Copy link
Contributor

darksylinc commented Aug 17, 2023

Although this is a bit subjective, I believe the motion vectors should be in the opposite direction as the current video.

The rationale is that this is a debugging feature and when you're debugging MV (motion vectors) bugs, you think in terms of where the current pixel used to be before, so the arrow should be pointed to where the pixel was.

Otherwise you're debugging a pixel from current frame with an arrow that indicates "look up", but there is nothing to look up. You need to look down, which is where that pixel used to be and that's where the information was.

In fact that's where the motion vector buffer is pointing to (the raw data numbers).

Without the debugging mindset, it sounds tempting to use the direction in the current video: it feels more intuitive that the arrow points to where the object is traveling to, as if it were some sort of gameplay feature.

However this is not a gameplay view, it is a debugging one. With the current arrows in the video, one has to mentally revert the direction which can be very exhausting and can lead to errors.

Not everyone has the same thought process though, so it can be a bit subjective.

@clayjohn
Copy link
Member

@darksylinc To be fair, Dario implemented the vectors pointing the other way originally and I asked him to switch the direction as I found it more intuitive the way he has it in the OP. I think seeing as both of you agree that the other direction is more intuitive, it makes sense to switch back

@DarioSamo
Copy link
Contributor Author

@darksylinc To be fair, Dario implemented the vectors pointing the other way originally and I asked him to switch the direction as I found it more intuitive the way he has it in the OP. I think seeing as both of you agree that the other direction is more intuitive, it makes sense to switch back

It's worth mentioning that going by Matias' argument of seeing the raw values, it should be clarified that these are the real raw values of the motion vectors. It did strike me a bit weird as usually the method I've seen (and the one FSR2 expects as well as I had to use a negative multiplier) is the way Matias described: the value that points to the previous pixel.

Sure enough if you look at taa_resolve.glsl, it becomes apparent it compensates for this direction in the same way I was originally doing as well:

// Get reprojected uv
vec2 uv_reprojected = uv - velocity;

And sure enough that is because scene_forward_clustered.glsl stores it that way:

motion_vector = position_uv - prev_position_uv;

Which results in a vector pointing the opposite direction.

Maybe it's worth evaluating to flip the sign of the value being stored and fixing it on the TAA pass instead. I feel that'd fit the convention of motion vectors that I've usually seen in most engines.

@Zireael07
Copy link
Contributor

Both directions have their merits, maybe expose this to editor settings?

Replaces the current method of showing the raw values of the motion vectors buffer to display a grid of lines instead with a new shader.
@DarioSamo DarioSamo force-pushed the debug-motion-vectors branch from c82aa95 to e7d3a7c Compare August 28, 2023 13:14
@DarioSamo DarioSamo marked this pull request as ready for review August 28, 2023 13:30
@DarioSamo DarioSamo requested a review from a team as a code owner August 28, 2023 13:30
@DarioSamo
Copy link
Contributor Author

DarioSamo commented Aug 28, 2023

Added the suggested changes and a red color for NaN values if they're detected as per Matias' suggestion.

This should be ready for reviewing.

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Looks great!

@clayjohn clayjohn modified the milestones: 4.x, 4.2 Aug 29, 2023
@clayjohn clayjohn added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Aug 29, 2023
@akien-mga akien-mga merged commit 9d74c24 into godotengine:master Aug 29, 2023
@akien-mga
Copy link
Member

Thanks!

@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Sep 21, 2023
@YuriSizov
Copy link
Contributor

This PR cannot be cherry-picked because it depends on #77085. We need to either approve #77085 for cherry-picking or make a version of this for 4.1 that doesn't use the debug effects class (or reimplements it for this feature without #77085).

@DarioSamo
Copy link
Contributor Author

DarioSamo commented Sep 21, 2023

I don't think we need to cherry-pick this anyway, it was just to make my life easier while debugging. Motion vector debug views aren't really used by the wider public yet. It can wait for 4.2 with FSR2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants