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

VideoCommon: fix graphics target texture names for efb/xfb #10881

Conversation

iwubcode
Copy link
Contributor

The efb/xfb names were not being calculated properly when defining actions that used them as textures. This made it impossible to write mods to skip (or technically move/scale) these draw calls.

I found this when trying to make a bloom replacement shader mod for Arc Rise Fantasia using the pp PR. I was removing bloom and applying the new bloom shader but a corner of the screen was unbloomed. Turns out the game was drawing in the corner to try and write over the place where it drew the bloom EFB copy. When I tried to get rid of that draw and I couldn't, I found this bug :)

@iwubcode iwubcode marked this pull request as draft July 22, 2022 03:58
@iwubcode iwubcode marked this pull request as ready for review July 22, 2022 05:02
@AdmiralCurtiss
Copy link
Contributor

I think you need to check the result of texture_info.find_first_of("_", letter_n_pos) against npos for the case where there's no _ after the n.

@iwubcode
Copy link
Contributor Author

@AdmiralCurtiss - it does work: https://godbolt.org/z/8MEh3TbE4

Though admittedly that's likely because npos is a very large value which means we will always erase wherever n is at. Still worth changing?

@AdmiralCurtiss
Copy link
Contributor

Yes, it feels a bit magic otherwise.

You also have a bug in here if the string starts with n : https://godbolt.org/z/PMoPWxfG1

@AdmiralCurtiss
Copy link
Contributor

AdmiralCurtiss commented Jul 30, 2022

A mid-string n not preceded by a _ is also not handled correctly...

@iwubcode
Copy link
Contributor Author

iwubcode commented Jul 30, 2022

Alright, I'll fix those up. Thanks for looking it over!

@iwubcode iwubcode force-pushed the graphics-mod-draw-fb-texture-names branch from b670f9f to d83de04 Compare July 30, 2022 22:06
@iwubcode
Copy link
Contributor Author

@AdmiralCurtiss - what do you think now?

I'm not sure why there is a FifoCI change..

@AdmiralCurtiss
Copy link
Contributor

AdmiralCurtiss commented Aug 6, 2022

I don't understand why you're checking for the prefix in the middle of the string and then rejecting the result if it's not at the start. Is there any reason to explicitly reject texture filenames that happen to have efb1 in the middle of their name? (a valid hexadecimal chain of characters, too!) Why not just do:

  const auto handle_fb_texture =
      [&texture_info](std::string_view type) -> std::optional<std::string> {
    const auto letter_n_pos = texture_info.find("_n");
    if (letter_n_pos == std::string::npos)
    {
      ERROR_LOG_FMT(VIDEO,
                    "Failed to load mod configuration file, value in 'texture_filename' "
                    "is {} without a count",
                    type);
      return std::nullopt;
    }

    const auto post_underscore = texture_info.find_first_of("_", letter_n_pos + 2);
    if (post_underscore == std::string::npos)
      return texture_info.erase(letter_n_pos, texture_info.size() - letter_n_pos);
    else
      return texture_info.erase(letter_n_pos, post_underscore - letter_n_pos);
  };

  if (texture_info.starts_with(EFB_DUMP_PREFIX))
    return handle_fb_texture("an efb");
  else if (texture_info.starts_with(XFB_DUMP_PREFIX))
    return handle_fb_texture("a xfb");
  return texture_info;

@iwubcode iwubcode force-pushed the graphics-mod-draw-fb-texture-names branch from d83de04 to a8fad6c Compare August 6, 2022 16:50
@iwubcode
Copy link
Contributor Author

iwubcode commented Aug 6, 2022

@AdmiralCurtiss - no real reason. I've been very reactionary to your requests instead of taking a step back and re-looking at the algorithm. The use of find("_n") is a smarter move. And I didn't have c++20 originally. Good stuff. From what I can tell, this handles all the scenarios.

Do note, that these don't support hashes (at least at the moment). They are size + format only. Still this is very clean. A little tempted to reject any that don't begin with efb,xfb, or tex1 but eh. Probably unnecessary. I've pushed it up.

@iwubcode iwubcode force-pushed the graphics-mod-draw-fb-texture-names branch from a8fad6c to 0b5f7d2 Compare August 6, 2022 17:01
@AdmiralCurtiss AdmiralCurtiss merged commit 45c4aa2 into dolphin-emu:master Sep 8, 2022
11 checks passed
@iwubcode iwubcode deleted the graphics-mod-draw-fb-texture-names branch September 8, 2022 21:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants