TiledQuick: Add border and grid over mapitem.#4392
Conversation
![kody-ai[bot]](https://avatars.githubusercontent.com/in/413034?s=60&v=4){kind=small}
UltraDagon
changed the title
|
Update: Added Grid over mapitem Created the following files to make the new GPU-rendered grid feature:
Other files were updated for the following reasons:
This addition was much larger than my last one, and I am definitely expecting to have gotten some of it wrong. Any and all feedback is appreciated! I added a few comments pointing out areas I'd like feedback on, if possible. Thank you! |
| id: mapGriditem | ||
| anchors.fill: mapItem | ||
|
|
||
| gridSize: mapItem.pixelToTileCoords(width, height); |
There was a problem hiding this comment.
For this property, I am not sure if this is the best way to get this information. Please let me know if there is a better way to get the grid size (X tiles by Y tiles) or tile size (in pixels).
There was a problem hiding this comment.
I guess there isn't yet. I think we'll need to somehow expose the loaded map as a EditableMap instance, which would make the necessary properties accessible from QML.
bjorn
left a comment
bjorn
left a comment
There was a problem hiding this comment.
This is quite a nice change, @UltraDagon, thanks for looking into these things!
I wondered a little about the purpose of the MapBorderItem as opposed to a standard Rectangle item, but I see the latter does not have a "cosmetic" mode so its border width would need to be inverse-adjusted to the scale to render at 1px. The specialized MapBorderItem might be the better solution.
I've tweaked the MapGridItem's fragment shader a little to place the grid on the edge between the tiles rather than on the tile. It really only makes difference in case of a width > 1 but it seems like this way we also didn't need the specific checks to disable the leftmost and topmost lines.
| id: mapGriditem | ||
| anchors.fill: mapItem | ||
|
|
||
| gridSize: mapItem.pixelToTileCoords(width, height); |
There was a problem hiding this comment.
I guess there isn't yet. I think we'll need to somehow expose the loaded map as a EditableMap instance, which would make the necessary properties accessible from QML.
|
Thank you for the feedback, Bjorn! I will work to get these changes finished within a few days. |
|
I ended up using mapItem's mMap to get the tile size information, and from that, I was able to get the grid size in tiles. |
bjorn
left a comment
bjorn
left a comment
There was a problem hiding this comment.
I've pushed some further tweaks and left you some more feedback. It's not quite ready for merging yet, but getting close. :-)
| material->mPixelWidth = width(); | ||
| material->mPixelHeight = height(); | ||
| material->mTileWidth = width() / mGridSize.x(); | ||
| material->mTileHeight = height() / mGridSize.y(); |
There was a problem hiding this comment.
This may divide by zero when mGridSize isn't set. But I think more interesting is that MapGridItem actually needs the tile size, not the grid size. So I'd suggest to rename mGridSize to mTileSize and set it accordingly, so that here we can just assign that to the material.
| auto *m = static_cast<const MapGridMaterial *>(other); | ||
| return (mColor == m->mColor && | ||
| qFuzzyCompare(mScale, m->mScale) && | ||
| qFuzzyCompare(mPixelWidth, m->mPixelWidth) ? 0 : 1); |
There was a problem hiding this comment.
I'm not sure this is doing what the compare function should be doing. The Qt docs talk about returning -1, 0 or 1, to establish a rendering order for materials that reduces state switches. Then again I'm also not entirely sure at the moment which properties are relevant in that case.
Why did you choose these properties and left out mPixelHeight, mTileWidth and mTileHeight?
There was a problem hiding this comment.
I had forgotten to add the rest of the comparable properties. Good catch, thank you!
| id: mapGriditem | ||
| anchors.fill: mapItem | ||
|
|
||
| gridSize: Qt.point(width / mapItem.tileSize().width, height / mapItem.tileSize().height); |
There was a problem hiding this comment.
Alright, so I just noticed the MapGridMaterial really just needs the tile size, so it's extra odd to do this calculation here. This should just be:
| gridSize: Qt.point(width / mapItem.tileSize().width, height / mapItem.tileSize().height); | |
| tileSize: mapItem.tileSize(); |
Once you've made the other suggested change. :-)
| var mapRelativeCoords = singleFingerPanArea.mapToItem(mapItem, singleFingerPanArea.mouseX, singleFingerPanArea.mouseY) | ||
| var tileCoords = mapItem.screenToTileCoords(mapRelativeCoords.x, mapRelativeCoords.y) | ||
| Math.floor(tileCoords.x) + ", " + Math.floor(tileCoords.y) | ||
| Math.floor(tileCoords.x) + ", " + Math.floor(tileCoords.y); |
There was a problem hiding this comment.
| Math.floor(tileCoords.x) + ", " + Math.floor(tileCoords.y); | |
| Math.floor(tileCoords.x) + ", " + Math.floor(tileCoords.y) |
Just suggesting to revert addition of semicolon. I'm open to changing to coding style, but then we'll do it consistently. :-)
There was a problem hiding this comment.
Sorry about that! I use that text area for testing and left it there by mistake.
|
@UltraDagon Hmm, you've opened this PR from your |
…lor property not updating border color
…es or color updates
…nsform node or preferably shader to make it better
… version check to tiledquick's condition to avoid crash when compiling with older qt versions
- Grid in between tiles - Fix file list order - Remove fileTags & overrideTags, relying on defaults
…culation instead of ScreenToTileCoords
* Added copyright headers attributing UltraDagon * Set flags in one call * Fixed emit from MapGridItem::setScale
|
Successfully split the maploader/editablemap changes into it's own pull request (#4520)! |
|
Would you like me to add the copyright headers to all files added in the pull request? |
I've added them to the source files mostly for consistency. I think now only the shaders do not have them, but you could add a brief comment to them as well. I personally wouldn't put the full copyright header there (it's actually a rather old habit by now I guess, which I haven't extended to QML files either). I think we could also merge this change as-is now. |
|
My apologies, I misread the commit and thought that only mapgridmaterial.h/cpp had the copyright headers. I agree that it would make more sense to only have the headers in the source files and leave them out of the other files. It should be good to merge, and I'll update my other PR to change the QML to use the new editablemap property. |
2 participants
Draws a simple 1px-width border around
MapItemusing the Qt Scene Graph. The border is drawn on top of the map, and the color is changeable withintiledquick/qml/main.qml.Draws a grid across the map. The color is changeable within
tiledquick/qml/main.qml. The grid's dash length, space length after dash, and thickness are changeable withinlibtiledquick/grid.frag.This is my first pull request, so I chose a small feature and wanted to practice mirroring the existing style and standards. This problem has also helped me gain some experience with QtQuick, and I plan on implementing the mouse-following grid-snapping object found in the default Tiled next.
As part of my attempt to imitate the existing style, I have kept my in-code comments to a minimum. Please let me know if including more comments is the better practice, and I can go back and add them.
Any and all feedback is appreciated!