Remove temporary artifacts from previous GFXR replays #388
Conversation
|
Notes:
|
angela28chen
force-pushed
the
clear-temp-artifacts
branch
from
0a8d396 to
038ed25
Compare
Maybe the warning could be a popup that appears when one of the artifact producing replay options is selected? Using the ShowErrorMessage(const std::string &err_msg); function. It could only popup once so the user isn't constantly dealing with popups if enabling more than one artifact producing replay option |
The thing is the artifacts will be wiped if the replay options that don't produce them are selected. So every time you replay it would affect the temporary artifacts. That's why I'm thinking a popup isn't a good option unless there's some kind of "don't show me this again" checkbox? Is that the popup option you mentioned? |
Personally I feel that a persistent warning text is better than a popup. But I guess this is not that important since using files is just a temp solution, and will soon be replaced by Elvis' comm infrastructure, and with that we dont have this issue. So for now i think you can keep the warning message as it is (maybe different font/color or maybe with warning signs like |
angela28chen
force-pushed
the
clear-temp-artifacts
branch
from
038ed25 to
2c084b8
Compare
|
@GrantComm @wangra-google I have updated this PR to also remove PM4 and force capture reload (which may be slower, but ensures the user sees the correct info from the most recent replay. In the future we might be able to speed it up and avoid re-loading the GFXR file with some refactoring) I also updated the warning text. PTAL (image in PR description) |
angela28chen
force-pushed
the
clear-temp-artifacts
branch
from
2c084b8 to
17f1c04
Compare
angela28chen
force-pushed
the
clear-temp-artifacts
branch
from
17f1c04 to
dc179a0
Compare
| @@ -139,7 +142,7 @@ private slots: | |||
| const int kDataRole = Qt::UserRole + 1; | |||
| const int kDefaultFrameCount = 3; | |||
| const std::string kDefaultReplayButtonText = "Replay"; | |||
| absl::StatusOr<std::string> m_capture_file_directory = ""; | |||
| absl::StatusOr<std::string> m_local_capture_file_directory = ""; | |||
There was a problem hiding this comment.
not really part of this change, but i feel it is a bit overkill to use absl::StatusOrstd::string for class members. When absl::StatusOrstd::string m_member is part of your object's state, it implies the object itself can exist in a "failed" state. This means that every single method that wants to use m_member must first check its status. This is repetitive and error-prone. You've just deferred the error handling from one place (creation) to everywhere (usage).
| } | ||
|
|
||
| // Run the gpu_time replay | ||
| std::string gpu_time_csv_stem = ""; |
There was a problem hiding this comment.
do we use gpu_time_csv_stem some where?
| WaitForReplay(*device); | ||
| qDebug() << "Loading gpu timing data: " << perf_counter_csv.string().c_str(); |
There was a problem hiding this comment.
a bit confused here: are we loading gpu time or perf counter here? should perf_counter_csv be replaced by something like gpu_time_csv()?
Since we moved to using the GFXR file load folder as the output folder for replay artifacts, currently there is an issue where subsequent replays will not have the proper artifacts displayed in the UI because we have improper cleanup and reload.
This PR addresses the issue for three files (profiling metrics CSV, gpu timing CSV, and PM4 capture) and adds a warning in the UI for the user to manually save any temporary files that they don't want cleared on the next replay. In the future when we have .dive files, more of this will be hidden from the user for ease of use.
Changes:
Minor changes: