New Hardware Renderer: Add tool brush to mapview.qml#4547
Conversation
…qml; Moved the cursor tile coords math to it's own function
4a8a8b4 to
0b03c73
Compare
Created new signal in mapview.qml to update mapeditor's tool mouse position (Temporary) Moved the ToolBrush map setter code to on mouse move Changed the ToolBrush map setter code to use the currently selected tool's brushItem->map()
|
Currently, toolBrushMap is set using Is this the recommended way for me to obtain this map? If so, I can go ahead and move it to be a public getter. I plan on integrating the quickBrushUpdated signal tomorrow, and then the colored overlays for using the different tools. |
This Depends is only there to make sure the plugin is compiled, but the plugin is loaded dynamically as a QML module.
bjorn
left a comment
There was a problem hiding this comment.
Nice to see things are coming together, but I had a lot of feedback. :-)
…efreshes upon map resize
initialize libtiledquick/mapitem's mEditableMap to nullptr so that it can be read during setmap removed unnecessary lines from mapview.qml renamed tilelayeritem's getlayer to layer added selectedTool boolean check to mapeditor::onquickmousecoordschanged changed dynamic_casts to qobject_casts
…ship in currentBrush() now applies to both renderers
…nges Added MapEditor::tileEditPreview() getter function Added AbstractTileTool::brushMap() getter function Moved toolBrushMap to a property var called toolPreviewMap to fix a garbage collector bug Need to add the Q_PROPERTY to mapeditor for the tileEditPreview. Pushed so that I can resume work on a difference machine.
…logic to be updated by mapview.qml
…the map as an argument
|
After linking a lot of these abstractTileTool signals, it might be better to link within the brushitems instead. Let me know what you think. Edit: I did this with the region changes, which, to do so, led to me having to change brushItem from a QGraphicsItem to a QGraphicsObject. |
Added RegionOverlay.qml to house the qml-based drawing of the overlay Added BrushItem::regionChanged signals Added BrushItem::setRegion and BrushItem::translateRegion functions Changed BrushItem to a QGraphicsObject (from QGraphicsItem) to support signals Fixed crash caused by toggling the New Hardware Renderer preference while a tile edit preview as active Minor code style changes in mapview.qml
|
After adding the regionOverlay and rendering it using QML geometry, I think it might be better to change the griditem from being shader-based to QML geometry-based, as the current shader floating-point errors cause the regionOverlay to be slightly offset from the grid. Let me know what you think 😄 |
| QHashIterator<TileLayer*, QRegion> ri(paintedRegions); | ||
| while (ri.hasNext()) { | ||
| ri.next(); | ||
| emit brushRegionChanged(ri.value()); |
There was a problem hiding this comment.
This doesn't look like the right place to emit a brush region changed signal. The ri.value() is the region in which an edit was applied.
There was a problem hiding this comment.
That makes sense. I've moved the brushRegionChanged to be mPreviewMap->modifiedTileRegion(), moved it out of the while loop, and changed the brushMapChanged to nullptr while dragging to paint.
…ounds Changed brushRegionOverlay to be visible/invisible when mouse is in/out of view Added validPolygons and invalidPolygons Q_PROPERTYs to RegionOverlay Changed StampBrush to hide tileEditPreview map upon drag Changed StampBrush's brush drag brushRegionChanged to be out of a for loop Added brushRegionChanged emits to AbstractTileSelectionTool and Eraser Removed unused AbstractTileTool::validRegion() getter
Added regionAlpha Q_PROPERTY to RegionOverlay Moved RegionOverlay's duplicate polygons code to it's own private function Changed toolBrush/regionOverlay to stay visible while mouse is pressed
Removed obsolete setTileEditPreview(nullptr)s Moved brushMapChanged and brushRegionChanged to more intuitive locations
|
I'm not super familiar with working with smart pointers and wanted to make sure that my most recent commit (a75bf94) is done correctly. From testing, I believe that it is properly freeing the EditableMap and SharedMap memory when they go out of scope, and mTileEditPreview should be the final owner of the toolBrush SharedMap. Let me know if I misunderstood anything while making this. Thanks! |
|
I also went ahead and undrafted the PR and updated the description since, once this is touched up and finalized, it should be a good stopping point before creating new PRs for other features. |
…erlay code tweaks
Talked a bit with Adam about this and added some safeguards to make sure this new constructor isn't misused in the future. I was also thinking that it could be possible to give MapItem the option to work how it used to, taking in a standard map, to avoid having to cast to an EditableMap. I'm not entirely sure, but I don't believe that the ToolBrush MapItem uses any EditableMap-specific features. |
The selected tool is only activated on a scene when there is a view using it, so the TILEDQUICK_LIB mouse handlers should not call into the tool when mViewWithTool is not set. Added MapEditor::activeTool, which checks this, and used it in place of selectedTool in these handlers.
bjorn
left a comment
There was a problem hiding this comment.
@UltraDagon It's really great to see this is actually working and to be able to edit with all those existing tools while using the new renderer!
I left a bunch of minor and some larger points of feedback, but it seems to me you're on track to get this merged by the end of the week. :-)
| cpp.includePaths: exportingProduct.sourceDirectory | ||
| } | ||
|
|
||
| Depends { name: "Qt.core" } |
There was a problem hiding this comment.
Why was this added here? I think that should be already covered by this line above:
Depends { name: "Qt"; submodules: ["quick","shadertools"]; versionAtLeast: "6.5" }
There was a problem hiding this comment.
Sorry about that, Qt Creator auto adds it whenever I add new files and I must have forgot to remove it.
| , mRegion(QRegion()) | ||
| , mMapRect(QRect()) |
There was a problem hiding this comment.
No need to explicitly write the initialization if it's the same as the default initialization, just leave it out in that case.
| } | ||
|
|
||
| Tiled::TileLayer *TileLayerItem::layer() { | ||
| return mLayer; |
There was a problem hiding this comment.
A good candidate for inlining. :-)
| { | ||
| if (!map) { | ||
| mTileEditPreview.reset(); | ||
| return; |
There was a problem hiding this comment.
I think this branch is missing the emit tileEditPreviewChanged()? You can also consider:
mTileEditPreview = map ? std::make_unique<EditableMap>(map) : nullptr;
emit tileEditPreviewChanged();(not sure if it works exactly like that, might need std::unique_ptr<EditableMap>() instead of nullptr to match the type)
There was a problem hiding this comment.
It seems like it should do the same thing. I used your suggested change.
| void setSelectedTool(AbstractTool *tool); | ||
|
|
||
| #ifdef TILEDQUICK_LIB | ||
| Q_INVOKABLE void setQuickMouseCoords(QPointF coords); |
There was a problem hiding this comment.
I guess as implemented this function should just be called quickMouseMoved for consistency.
| mRegion = region; | ||
| } | ||
|
|
||
| void BrushItem::translateRegion(const QPoint &p) |
There was a problem hiding this comment.
What is this function for? Also setRegion seems superfluous.
There was a problem hiding this comment.
setRegion and translateRegion were remnants from when I was getting signals from brushItem changes, so they're no longer needed, but I had left them in anyway in case they were needed in the future to more easily send signals. I'll remove them since they don't seem necessary or hard to re-add if needed in the future.
| id: regionOverlay | ||
|
|
||
| property var scale | ||
| // TODO: Draw only visible area |
There was a problem hiding this comment.
I guess we can remove that TODO for now, because it's unclear how that's supposed to work and also whether it will be more efficient anyway.
Hopefully the Shape item will already do some optimizations, but even if it doesn't, adjusting its shapes based on the visible area would also have a certain performance overhead.
| var tileCoords = mapItem.screenToTileCoords(mapRelativeCoords.x, mapRelativeCoords.y) | ||
|
|
||
| return tileCoords |
There was a problem hiding this comment.
Let's shorten this a bit. :-)
| var tileCoords = mapItem.screenToTileCoords(mapRelativeCoords.x, mapRelativeCoords.y) | |
| return tileCoords | |
| return mapItem.screenToTileCoords(mapRelativeCoords.x, mapRelativeCoords.y) |
| var mapRelativeCoords = singleFingerPanArea.mapToItem(mapItem, singleFingerPanArea.mouseX, singleFingerPanArea.mouseY) | ||
| return mapRelativeCoords |
There was a problem hiding this comment.
| var mapRelativeCoords = singleFingerPanArea.mapToItem(mapItem, singleFingerPanArea.mouseX, singleFingerPanArea.mouseY) | |
| return mapRelativeCoords | |
| return singleFingerPanArea.mapToItem(mapItem, singleFingerPanArea.mouseX, singleFingerPanArea.mouseY) |
| WangBrush *mWangBrush; | ||
| EditPolygonTool *mEditPolygonTool; | ||
|
|
||
| std::unique_ptr<EditableMap> mTileEditPreview; |
There was a problem hiding this comment.
I think it's a problem to use std::unique_ptr<EditableMap> here and also pass the EditableMap* into the QML context, from where it is passed on to MapItem. We're probably kind of lucky when the unique_ptr deletes the EditableMap instance, that the MapItem no longer references that pointer.
When we create the EditableMap instance, actually its EditableObject parent constructor will set it as the Object::mEditable. That means it probably suffices to just store a SharedMap mTileEditPreview here, and we can create its EditableMap instance on-demand in MapEditor::tileEditRegion, using EditableObject::find(mTileEditPreview.get()) to reuse an existing editable instance when possible.
Removed unnecessary Qt.core dependency from libtiledquick.qbs Removed redundant RegionOverlay mRegion and mMapRect initializations Changed TileLayerItem::layer to be inline Renamed MapEditor's setQuickMouseCoords to quickMouseMoved Removed redundant EditableMap mSharedMap default assignment Removed unnecessary setRegion and translateRegion functions from BrushItem Removed TODO comment from RegionOverlay Shortened mapRelativeCoords function in mapview.qml Removed unused cursorTileCoords function from mapview.qml Changed mTileEditPreview to be a SharedMap Changed tileEditPreview to create the new editableMap instance on request
This adds Tiled's existing basic tool functionality to the New Hardware Renderer's mapview.qml.
Adds a toolBrush MapItem to mapview.qml.
Adds a new RegionOverlay.qml element used to highlight some region of the map.
Adds a selectedRegion RegionOverlay for tile-selection-based tools.
Adds a brushRegion RegionOverlay for the ToolBrush's edit preview region.
Adds mouse functionality to singleFingerPanArea in mapview.qml.
Changes mapItem to update upon tool usage.
Adds a new EditableMap constructor, allowing for the usage of SharedMaps.
Adds sizeChange and tileSizeChanged signals to EditableMap.
Lesser changes:
Abstracted setRegion and translateRegion functions in BrushItem. Not required for this PR, but may be useful in the future to signal changes at the BrushItem level.