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

MemoryViewWidget refactor memory table #11111

Merged

Conversation

TryTwo
Copy link
Contributor

@TryTwo TryTwo commented Oct 1, 2022

Everything in the table was being re-created on every update. Future work needs the table to persist.

Added CreateTable(), move all constants and semi-constants into it. It is called when the table size changes, font changes, or the displayed data type changes.

Update() now updates address values and calls other update functions. Ideally it would only be called when addresses change (scroll, setting a different target address, etc) and after CreateTable(). Cleaning up update logic can be done in another PR, as it depends on MemoryWidget.

UpdateColumns() could be renamed to UpdateDataColumnValues() because it's for updating displayed memory values. This function is smaller than before, which is useful for future PRs. Many constants being re-created were moved. The address is now pulled from previous calculated and updated addresses. It also seemed easier to move the switch section to a new function, because it didn't need to share as much as before.

MISC_COLUMNS is a new constexpr which was being used in many places. It may change in the future, so good to keep track of it.

constexpr auto USER_ROLE_IS_ROW_BREAKPOINT_CELL = Qt::UserRole; -- Feels a bit overkill. It's synonymous with item->column() == 0; But I didn't change it. It could be constexpr int BREAKPOINT_COLUMN = 0; then column() == BREAKPOINT_COLUMN so it wouldn't need to be set on every item.

Source/Core/DolphinQt/Debugger/MemoryViewWidget.cpp Outdated Show resolved Hide resolved
Source/Core/DolphinQt/Debugger/MemoryViewWidget.cpp Outdated Show resolved Hide resolved
Source/Core/DolphinQt/Debugger/MemoryViewWidget.cpp Outdated Show resolved Hide resolved
@TryTwo
Copy link
Contributor Author

TryTwo commented Oct 2, 2022

accessors->IsValidAddress(addr) checks all the conditions we need at once (game is running, memory address exists, and if it exists it has a value)

/edit Actually still need the Type Null check to exclude the address column. Null should only be representing the BP and Address columns now.

@TryTwo
Copy link
Contributor Author

TryTwo commented Oct 2, 2022

Because the table isn't being re-created on update, the item_selected pointer will hold any new values if they change. So there's no need for the refresh check. Don't think there's a case where the item pointer can become invalid while the context menu is open, but if so I could add a check to see if it still exists.

Copy link
Contributor

@sepalani sepalani left a comment

Choose a reason for hiding this comment

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

I haven't tested this PR yet but here are some suggestions that might slightly improve performance.

Source/Core/DolphinQt/Debugger/MemoryViewWidget.cpp Outdated Show resolved Hide resolved
Source/Core/DolphinQt/Debugger/MemoryViewWidget.cpp Outdated Show resolved Hide resolved
Source/Core/DolphinQt/Debugger/MemoryViewWidget.cpp Outdated Show resolved Hide resolved
@TryTwo
Copy link
Contributor Author

TryTwo commented Oct 20, 2022

I settled on using m_data_columns, because if Symbols are re-added, it will be a new column on the right-side that will throw the old method off.

I don't know if clone() is faster when creating a table, but I thought it looked a little nicer. Removed the initial address, because Update() will do that.

Copy link
Contributor

@sepalani sepalani left a comment

Choose a reason for hiding this comment

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

Tested and working as expected. Beside the lint error the code LGTM too.

Don't re-create the table on every update.
@TryTwo
Copy link
Contributor Author

TryTwo commented Oct 24, 2022

Thanks! I finished fixing it.

@AdmiralCurtiss AdmiralCurtiss merged commit 89bc164 into dolphin-emu:master Nov 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants