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

Can't directly modify points of Line2D #62857

Closed
KoBeWi opened this issue Jul 9, 2022 · 6 comments · Fixed by #72398
Closed

Can't directly modify points of Line2D #62857

KoBeWi opened this issue Jul 9, 2022 · 6 comments · Fixed by #72398

Comments

@KoBeWi
Copy link
Member

KoBeWi commented Jul 9, 2022

Godot version

d26442e

System information

Windows 10 x64

Issue description

Broken:

extends Line2D
func _process(delta: float) -> void:
	points[0].y += 10 * delta

Also broken:

extends Line2D
func _process(delta: float) -> void:
	points[0] += Vector2.DOWN * 10 * delta

Workaround:

extends Line2D
func _process(delta: float) -> void:
	var p = Array(points)
	p[0].y += 10 * delta
	points = PackedVector2Array(p)

I remember it was fine a month ago or so.

Probably not related to Line2D, but this where I noticed this first.

Steps to reproduce

  1. Add Line2D
  2. Add some script
  3. Try to change points of Line2D

Minimal reproduction project

No response

@KoBeWi KoBeWi added this to the 4.0 milestone Jul 9, 2022
@Chaosus
Copy link
Member

Chaosus commented Jul 9, 2022

Why simply not use set_point_position(0, Vector2(get_point_position(0).x * 10 * delta, get_point_position(0).y))?

@KoBeWi
Copy link
Member Author

KoBeWi commented Jul 9, 2022

Oh I didn't see these methods 🤔
Still, it was working before, so it's a regression (it broke my code).

@cdemirer
Copy link
Contributor

cdemirer commented Jul 11, 2022

This probably happens due to the weirdness of Packed Array passing behavior.

Since #62462, the VM doesn't unnecessarily set values back to parent when something was modified in-place (shared stuff like Array, Dictionary, etc.).

However, Packed Arrays are edge cases. They are currently shared types within the boundaries of GDScript (they are assigned to vars by reference, and they pass to GDScript functions just like Arrays do), but they are copied when passing between GDScript and the C++ API. See #36492 (comment).

The optimization in #62462 regards Packed Arrays as shared types (which they kinda are), however Line2D's points is not exactly shared, despite being accessed as a Packed Array.

@vnen
Copy link
Member

vnen commented Jul 13, 2022

So, we were discussing this in the GDScript meeting, and one potential solution would be to update the JUMP_IF_SHARED instruction to take into account whether the property is from a native engine type or from a script and jump accordingly.

In any case, we need to update Variant::is_shared() to remove the packed arrays, since they are copied within the engine. This function isn't used anywhere else right now but it might create trouble in the future.

@Maran23
Copy link
Contributor

Maran23 commented Nov 10, 2022

This is also partly broken for 3.x already:
This does not work:

for point in line_x.points:
    point.y = position.y

This however works at least:

line_x.points[0].y = position.y
line_x.points[1].y = position.y

So there is probably more.

@Lippanon
Copy link

Still present in v4.0.beta.custom_build [91713ce]. Can't edit a CSGPolygon3D's polygon directly (PackedVector2Array):

csgpolygon3d_ref.polygon.set(1, Vector2(1.0, 1.0)) # Doesn't change the value
print(csgpolygon3d_ref.polygon[1])
csgpolygon3d_ref.polygon[1] = Vector2(1.0, 1.0) # Doesn't change the value
print(csgpolygon3d_ref.polygon[1])

The same workaround of setting a new PackedVector2Array works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

6 participants