Token Manager (Qt6): default checkboxes unchecked; token size 32px #400
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…hing - Introduced a new CHARACTER_TOKEN column to the Group Manager table. - Integrated TokenManager for resolving and loading character token images. - Implemented fallback logic and QPixmapCache in TokenManager to reduce redundant image loading. - Added GroupDelegate logic to render icons, with default size set to 32x32 pixels. - Suppressed repeated debug output now that caching is effective. - Layout and header adjusted to support the new Icon column without disrupting existing columns. Foundation laid for future enhancements, including token toggle and dynamic resizing.
* Uploads token PNGs once and registers them in the live OpenGL renderer * Player token drawn immediately; mounts/NPCs hidden in player???s room * Prevents duplicate overlays and black fallback
… icon; paint background first; move header resize after model set; recalc token column/rows on icon-size change
…zeHint matches scaled icon; revert all-columns auto-resize; keep per-column header setup after model set
…icon; sizeHint matches scaled icon; revert all-columns auto-resize; keep per-column header setup after model set" This reverts commit bf166d2.
…o scaled icon; paint background first; move header resize after model set; recalc token column/rows on icon-size change" This reverts commit a1bde36.
… centering)" This reverts commit 4f3de6d.
Reviewer's GuideThis PR adds a new TokenManager to handle custom character icons, integrates token rendering into the group list and map (including NPC ghost tokens), introduces new group preferences (show/hide tokens, map tokens, NPC ghosts, and configurable token size with defaults unchecked and 32px), refactors GroupModel character insertion/removal to use a helper and ghost registry, and updates build and docs accordingly. Sequence diagram for updating character token and map renderingsequenceDiagram
actor User
participant GroupWidget
participant TokenManager
participant MapCanvas
User->>GroupWidget: Set icon for character
GroupWidget->>TokenManager: getToken(key)
GroupWidget->>GroupWidget: slot_updateLabels()
GroupWidget->>MapCanvas: sig_characterUpdated(character)
MapCanvas->>TokenManager: getToken(key)
MapCanvas->>MapCanvas: slot_requestUpdate()
MapCanvas->>MapCanvas: paintCharacters() (uses token icon)
Entity relationship diagram for new group manager settingserDiagram
GROUP_MANAGER_SETTINGS {
bool showTokens
bool showMapTokens
int tokenIconSize
bool showNpcGhosts
QMap tokenOverrides
}
TOKEN_OVERRIDES {
QString characterName
QString tokenKey
}
GROUP_MANAGER_SETTINGS ||--o{ TOKEN_OVERRIDES : contains
Class diagram for new and updated token management classesclassDiagram
class TokenManager {
+QPixmap getToken(key: QString)
+static QString overrideFor(displayName: QString)
+const QMap<QString, QString> &availableFiles()
+MMTextureId textureIdFor(key: QString)
+MMTextureId uploadNow(key: QString, px: QPixmap)
+void rememberUpload(key: QString, id: MMTextureId, tex: SharedMMTexture)
+SharedMMTexture textureById(id: MMTextureId) const
-void scanDirectories()
-QMap<QString, QString> m_availableFiles
-QFileSystemWatcher m_watcher
-QMap<QString, QString> m_tokenPathCache
-QPixmap m_fallbackPixmap
-QHash<QString, MMTextureId> m_textureCache
-QVector<SharedMMTexture> m_ownedTextures
}
class CGroupChar {
-QString m_characterToken
+QString getDisplayName() const
+void setCharacterToken(tokenPath: QString)
+const QString &getCharacterToken() const
+inline bool isMount() const
}
class GhostInfo {
+QString tokenKey
}
class GhostRegistry {
}
TokenManager <.. CGroupChar : uses
CGroupChar <.. GhostRegistry : used for ghost tokens
GhostRegistry <.. GhostInfo : contains
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/group/groupwidget.cpp:850` </location>
<code_context>
+ emit sig_characterUpdated(selectedCharacter);
});
+ connect(m_table, &QAbstractItemView::clicked, this, &GroupWidget::showContextMenu);
+
connect(m_group, &Mmapper2Group::sig_characterAdded, this, &GroupWidget::slot_onCharacterAdded);
</code_context>
<issue_to_address>
**question:** Context menu is now shown on click, which may interfere with row selection.
Directly connecting showContextMenu to the clicked signal may disrupt standard row selection or double-click behavior. Please review if this impacts usability and consider limiting context menu activation to right-click or a specific action.
</issue_to_address>
### Comment 2
<location> `src/group/groupwidget.cpp:760` </location>
<code_context>
+ const int row = std::max(icon, m_table->fontMetrics().height() + 4);
+ m_table->verticalHeader()->setDefaultSectionSize(row);
+ m_table->setIconSize(QSize(icon, icon));
m_center = new QAction(QIcon(":/icons/roomfind.png"), tr("&Center"), this);
connect(m_center, &QAction::triggered, this, [this]() {
</code_context>
<issue_to_address>
**suggestion:** Icon size and row height are now dynamically set, but may not handle DPI scaling.
Please verify that the sizing logic works correctly on high-DPI displays and with system scaling enabled across platforms.
Suggested implementation:
```cpp
// Minimize row height, handle DPI scaling
const int icon = getConfig().groupManager.tokenIconSize;
qreal scale = m_table->devicePixelRatioF();
const int scaledIcon = std::round(icon * scale);
const int scaledRow = std::max(scaledIcon, std::round((m_table->fontMetrics().height() + 4) * scale));
m_table->verticalHeader()->setDefaultSectionSize(scaledRow);
m_table->setIconSize(QSize(scaledIcon, scaledIcon));
```
If you want to support per-screen DPI scaling (multi-monitor setups), consider using `QScreen *screen = m_table->screen();` and `screen->devicePixelRatio()` instead of `m_table->devicePixelRatioF()`. Also, ensure that your icons are provided at multiple resolutions (e.g., SVG or @2x PNGs) for best appearance.
</issue_to_address>
### Comment 3
<location> `src/display/Characters.cpp:356-359` </location>
<code_context>
}
+ // ── draw map-tokens underneath the coloured overlay ──
+ for (size_t q = 0; q < m_charTokenKeys.size(); ++q) {
+ const size_t base = q * 4;
+ if (base + 3 >= m_charTokenQuads.size())
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Token quad rendering assumes exactly four vertices per token.
If quad generation logic changes, mismatched vertex counts could cause rendering issues. Adding assertions or error handling would help detect these problems early.
```suggestion
for (size_t q = 0; q < m_charTokenKeys.size(); ++q) {
const size_t base = q * 4;
// Ensure each token quad has exactly four vertices
Q_ASSERT_X(base + 3 < m_charTokenQuads.size(), "Token quad rendering", "Each token quad must have exactly four vertices.");
if (base + 3 >= m_charTokenQuads.size()) {
qWarning("Token quad rendering: Mismatched vertex count for token quad %zu (expected 4 vertices per token)", q);
break;
}
```
</issue_to_address>
### Comment 4
<location> `src/group/CGroupChar.h:125` </location>
<code_context>
#undef X_DECL_GETTERS_AND_SETTERS
+ /* ---------- temporary helper until GMCP flags real mounts ---------- */
+ inline bool isMount() const
+ {
+ return isNpc(); // treat every NPC as a “mount” for now
</code_context>
<issue_to_address>
**nitpick:** isMount() currently treats all NPCs as mounts, which may be misleading.
Since isMount() currently returns true for all NPCs, please add a comment or TODO indicating this is a temporary solution and will be updated when GMCP support is available.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| emit sig_characterUpdated(selectedCharacter); | ||
| }); | ||
|
|
||
| connect(m_table, &QAbstractItemView::clicked, this, &GroupWidget::showContextMenu); |
There was a problem hiding this comment.
question: Context menu is now shown on click, which may interfere with row selection.
Directly connecting showContextMenu to the clicked signal may disrupt standard row selection or double-click behavior. Please review if this impacts usability and consider limiting context menu activation to right-click or a specific action.
| const int row = std::max(icon, m_table->fontMetrics().height() + 4); | ||
|
|
||
| m_table->verticalHeader()->setDefaultSectionSize(row); | ||
| m_table->setIconSize(QSize(icon, icon)); |
There was a problem hiding this comment.
suggestion: Icon size and row height are now dynamically set, but may not handle DPI scaling.
Please verify that the sizing logic works correctly on high-DPI displays and with system scaling enabled across platforms.
Suggested implementation:
// Minimize row height, handle DPI scaling
const int icon = getConfig().groupManager.tokenIconSize;
qreal scale = m_table->devicePixelRatioF();
const int scaledIcon = std::round(icon * scale);
const int scaledRow = std::max(scaledIcon, std::round((m_table->fontMetrics().height() + 4) * scale));
m_table->verticalHeader()->setDefaultSectionSize(scaledRow);
m_table->setIconSize(QSize(scaledIcon, scaledIcon));
If you want to support per-screen DPI scaling (multi-monitor setups), consider using QScreen *screen = m_table->screen(); and screen->devicePixelRatio() instead of m_table->devicePixelRatioF(). Also, ensure that your icons are provided at multiple resolutions (e.g., SVG or @2x PNGs) for best appearance.
Comment on lines
+356
to
+359
| for (size_t q = 0; q < m_charTokenKeys.size(); ++q) { | ||
| const size_t base = q * 4; | ||
| if (base + 3 >= m_charTokenQuads.size()) | ||
| break; |
There was a problem hiding this comment.
suggestion (bug_risk): Token quad rendering assumes exactly four vertices per token.
If quad generation logic changes, mismatched vertex counts could cause rendering issues. Adding assertions or error handling would help detect these problems early.
Suggested change
| for (size_t q = 0; q < m_charTokenKeys.size(); ++q) { | |
| const size_t base = q * 4; | |
| if (base + 3 >= m_charTokenQuads.size()) | |
| break; | |
| for (size_t q = 0; q < m_charTokenKeys.size(); ++q) { | |
| const size_t base = q * 4; | |
| // Ensure each token quad has exactly four vertices | |
| Q_ASSERT_X(base + 3 < m_charTokenQuads.size(), "Token quad rendering", "Each token quad must have exactly four vertices."); | |
| if (base + 3 >= m_charTokenQuads.size()) { | |
| qWarning("Token quad rendering: Mismatched vertex count for token quad %zu (expected 4 vertices per token)", q); | |
| break; | |
| } |
| #undef X_DECL_GETTERS_AND_SETTERS | ||
|
|
||
| /* ---------- temporary helper until GMCP flags real mounts ---------- */ | ||
| inline bool isMount() const |
There was a problem hiding this comment.
nitpick: isMount() currently treats all NPCs as mounts, which may be misleading.
Since isMount() currently returns true for all NPCs, please add a comment or TODO indicating this is a temporary solution and will be updated when GMCP support is available.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Updates Token Manager to Qt 6. Sets checkboxes default to unchecked and tokenSizeComboBox default to 32 px. Built and validated in alt CI; identical commits here.
Summary by Sourcery
Introduce full token management and rendering for characters in both group and map views, including custom overrides, ghost tokens for removed NPCs, and new user preferences defaulting to 32px icons.
New Features:
Enhancements:
Build:
Documentation: