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

Setting object.polygon in script makes all points (0,0) #3845

Closed
IoriBranford opened this issue Nov 18, 2023 · 5 comments
Closed

Setting object.polygon in script makes all points (0,0) #3845

IoriBranford opened this issue Nov 18, 2023 · 5 comments
Labels
bug Broken behavior.

Comments

@IoriBranford
Copy link

IoriBranford commented Nov 18, 2023

Describe the bug
When a script assigns a new polygon to a MapObject, all the polygon points will end up as (0,0).

To Reproduce
Steps to reproduce the behavior:

  1. Install the test script
/// <reference types="@mapeditor/tiled-api" />

const TouchPolygon = tiled.registerAction("Touch Polygon", (action) => {
    let asset = tiled.activeAsset
    let selected = asset.selectedObjects
    if (!selected) {
        return
    }
    
    for (let object of selected) {
        let polygon = object.polygon
        if (!polygon)
            continue
        asset.macro("Touch Polygon", () => {
            // Doc says: "To modify the polygon of a MapObject, change or set up the polygon array and then assign it to MapObject.polygon"
            object.polygon = polygon.map((p) => ({ x: p.x, y: p.y }))
            tiled.log(polygon)
            tiled.log(object.polygon)
        })
    }
})
TouchPolygon.text = "Touch Polygon"

tiled.extendMenu("MapView.Objects", [
    { action: "Touch Polygon" }
])
  1. Create a polygon or polyline object
  2. Right-click on the object and select "Touch Polygon"
  3. See console output

Expected behavior
The polygon is unchanged and two identical point arrays are output to console.

Actual behavior
The polygon collapses as all its points are zeroed. This is confirmed by console output, in which the second array has the same number of points but they are all QPointF(0,0).

Specifications:

  • OS: Linux Mint Vera
  • Tiled Version: 1.10.2
@IoriBranford IoriBranford added the bug Broken behavior. label Nov 18, 2023
@eishiya
Copy link
Contributor

eishiya commented Nov 18, 2023

The Polygon array needs to be an array of Points. { x: p.x, y: p.y } is not a Point, it's a JS object that looks like a Point, so the assignment probably just doesn't work, and each point gets set to a default value. Try creating the Polygon points with Qt.point(p.x, p.y) instead.

The documentation for MapObject.polygon is a little misleading, it makes it sound like any object with x and y properties will do, but it has to be specifically a point.

@IoriBranford
Copy link
Author

Qt.point does work, but I remember JS object points working in previous Tiled versions. (Don't have time to find which version ATM.) Thanks

@eishiya
Copy link
Contributor

eishiya commented Nov 18, 2023

I think it was the change from Qt5 to Qt6 that made typing more strict, not a change to Tiled directly.

@bjorn
Copy link
Member

bjorn commented Nov 20, 2023

While Qt 6 has become more strict for functions called from JS, this is likely also the result of change 4a0f569 made for Tiled 1.10.2, where I changed the QJSValue polygonValue argument it took previously to a const QPolygonF &polygon, since that appeared to work just as well and avoids a bunch of annoying glue code.

However, I wasn't aware this would then only work for objects created using Qt.point as opposed to arbitrary objects having an x and y property, as previously supported. So now I wonder if I shouldn't just re-enable the annoying glue code also for Qt 6 builds for the setter function or whether there is any advantage of the const QPolygonF &polygon version (other than performance, which is likely better but unlikely to matter).

I'd stick with the more strictly typed one if it resulted in proper type errors, but as it stands there is apparently a silent failure to do what would be expected, which is worse.

@IoriBranford
Copy link
Author

So now I wonder if I shouldn't just re-enable the annoying glue code also for Qt 6 builds for the setter function

I agree with this on the principle of least surprise, since JavaScript isn't normally expected to be strict on types.

@bjorn bjorn closed this as completed in 97ec0bf Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Broken behavior.
Projects
None yet
Development

No branches or pull requests

3 participants