[Feature]: Drawing functions for creating and modifying

[bubblobill]

Describe the Problem

Drawing functions are limited in that they only permit rudimentary modifcation of existing drawings generated through the GUI.
Functionality should also include the creation and modification of the geometry.

The Solution you'd like

Expand existing drawing functions to include generating and modifying the underlying geometry.

Alternatives that you've considered.

No response

Additional Context

No response

[bubblobill]

#1410 #2743 #3189 #4867 #4584

[kwvanderlinde]

I tested the basics (shape.create() + shape.draw()) and have some feedback.

---

In the "arc" shape.create() implementation:

• The type ("pie", "chord", "open") is not used. Well, it's set at first internally, but is not sent to any clients (even the local client), so it always ends up as the default ("open").
• The "y" parameter is used as the "h". Similar to the previous point, internally it starts out correctly, but clients mix up the values when deserializing ArcDto.

---

When an unrecognized shape type is provided to shape.create(), the user is only given a cryptic error:

"Unsupported Operation" error in processing "polygon-wonky" for argument number 3 to function "shape.create".
Error trace : ShapeFunctions@global

They should be told something more specific like Unrecognized shape type "polygon-wonky" provided to shape.create()".

---

For the shape types that take a JSON object of parameters (e.g., "arc", "line", etc), if a field is missing then the user receives an untranslated error message:

macro.function.general.wrongNumFields
Error trace : ShapeFunctions@global

But I don't think the message would be clear enough even if translated. I'd rather it tell me what is missing, as is done when a field is mispelled:

Argument 4 in function shape.create is missing the required key "x". Required keys are; "x, w, h, start, type, extent, y"
Error trace : ShapeFunctions@global

---

If the "numpoints" provided to the "polygon" shape is more than the number of x or y coordinates, the user gets a large dump in chat:

java.lang.IndexOutOfBoundsException: npoints > xpoints.length || npoints > ypoints.length error executing expression vShapes = json.append("[]", shape.create("arc-pie", "TOKEN", true, "arc", json.set("{}", "x", 0, "y", 0, "w", 100, "h", 100, "start", 0, "extent", 90, "type", "pie" )), shape.create("arc-chord", "TOKEN", true, "arc", json.set("{}", "x", 0, "y", 200, "w", 100, "h", 100, "start", 0, "extent", 90, "type", "chord" )), shape.create("arc-open", "TOKEN", true, "arc", json.set("{}", "x", 0, "y", 400, "w", 100, "h", 100, "start", 0, "extent", 90, "type", "open" )), shape.create("line", "TOKEN", true, "line", json.set("{}", "x1", 200, "y1", 0, "x2", 300, "y2", 100 )), shape.create("quadcurve", "TOKEN", true, "quadcurve", json.set("{}", "x1", 200, "y1", 200, "ctrlx", 230, "ctrly", 270, "x2", 300, "y2", 300 )), shape.create("cubiccurve", "TOKEN", true, "cubiccurve", json.set("{}", "x1", 200, "y1", 400, "ctrlx1", 200, "ctrly1", 430, "ctrlx2", 300, "ctrly2", 470, "x2", 300, "y2", 500 )), shape.create("ellipse", "TOKEN", true, "ellipse", json.set("{}", "x", 400, "y", 0, "w", 100, "h", 100 )), shape.create("rectangle", "TOKEN", true, "rectangle", json.set("{}", "x", 400, "y", 200, "w", 100, "h", 100 )), shape.create("roundrectangle", "TOKEN", true, "roundrectangle", json.set("{}", "x", 400, "y", 400, "w", 100, "h", 100, "arcw", 50, "arch", 25 )), shape.create("pentagon", "TOKEN", true, "polygon", json.set("{}", "xpoints", json.append("[]", 400, 450, 500, 450, 400), "ypoints", json.append("[]", 600, 600, 650, 700, 700), "numpoints", 5 )), shape.create("polygon-wonky", "TOKEN", true, "polygon", json.set("{}", "xpoints", json.append("[]", 400, 450, 500, 450, 400), "ypoints", json.append("[]", 800, 800, 850, 900), "numpoints", 5 )) ).
Error trace : ShapeFunctions@global

We should do our own bounds checking and provide a clear and succinct message if there is a problem. I would suggest enforcing that the "xpoints" and "ypoints" arrays are the same length, and removing the "numpoints" field altogether as it can be inferred from "xpoints" and "ypoints".

Another thought, incompatible with the above, is that users would probably find it more intuitive to supply an array of coordinate pairs rather than a pair of coordinate arrays. E.g., allow one of these forms:

{
...
"points": [[0, 0], [100, 0], [100, 100], [0, 100]]
...
}

...
"points": [{"x": 0, "y": 0}, {"x": 100, "y": 0}, {"x": 100, "y": 100}, {"x": 0, "y": 100}]
...

[kwvanderlinde]

Also some implementation notes. I see you've added support for ExtendedGeneralPath and even extended it to ExtendedGeneralPath_Double. But these are handled very inconsistently. If I make a "path" shape, it follows this lifecycle:

1. It is born as an ExtendedGeneralPath (single precision)
2. It is serialized to PathShapeDto to be sent to clients (including current client). But this serialization "flattens" the arc segments into cubics, so we never write out an ArcToSegment.
3. On the receiving clients, that PathShapeDto is initially deserialized to an ExtendedGeneralPath_Double (double precision). So we've "gained" extra precision.
4. Immediately that ExtendedGeneralPath_Double is converted to a Path2D.Double which has no arc support. Luckily we lost our arcs in (2) so it's not a problem!

At the end of it all, it looks like campaigns don't store any ExtendedGeneralPath or ExtendedGeneralPath_Double, just the arc-less Path2D.Double.

I actually think that outcome is for the best, at least for now. I'm a little sketched by the idea of serializaing a 3rd party type into our campaign XML, especially for the sake of an experimental feature. Once it's in there we're stuck with it. So I would suggest we explicitly avoid that, and also represent paths more consistently. I.e., use ExtendedGeneralPath when first creating the path so we get arc support, but then immediately reduce that to a plain Path2D.Double. And then have ShapeDrawable only support Path2D.Double.

[bubblobill]

#5236

• The type ("pie", "chord", "open") is not used. Well, it's set at first internally, but is not sent to any clients (even the local client), so it always ends up as the default ("open").
• The "y" parameter is used as the "h". Similar to the previous point, internally it starts out correctly, but clients mix up the values when deserializing ArcDto.

No great shock. I hate Dtos. They are tres awkward.
Think I fixed it.

When an unrecognized shape type is provided to shape.create(), the user is only given a cryptic error:

"Unsupported Operation" error in processing "polygon-wonky" for argument number 3 to function "shape.create".
Error trace : ShapeFunctions@global

They should be told something more specific like Unrecognized shape type "polygon-wonky" provided to shape.create()".

Sheesh. You create one decent error message and suddenly you are expected to have made all error messages nice. Also, I dispute the cryptic. You have been clearly informed that "polygon-wonky" doesn't work. Additionally, you have been given an error type that is helpful for devs in bug-hunting.

We actually need to cut down all the error messages to be less specific. This way we can recycle them instead of constantly adding new strings for translators to get confused about. Something like Unrecognized value "polygon-wonky" provided to shape.create()". makes more sense.

Am upgrading the error message.

macro.function.general.unsupportedOperation        = "Unsupported Operation" error in processing "{2}" for argument number {1} to function "{0}".
macro.function.general.unsupportedOperation.verbose= "Unsupported Operation" error in processing "{2}" for argument number {1} to function "{0}". Expected one of: {3}

For the shape types that take a JSON object of parameters (e.g., "arc", "line", etc), if a field is missing then the user receives an untranslated error message:

Changed this to just spit out the first missing key.

Error trace : ShapeFunctions@global

If the `"numpoints"` provided to the `"polygon"` shape is more than the number of x or y coordinates, the user gets a large dump in chat:

java.lang.IndexOutOfBoundsException: npoints > xpoints.length || npoints > ypoints.length error executing expression vShapes = json.append("[]", shape.create("arc-pie", "TOKEN", true, "arc", json.set("{}", "x", 0, "y", 0, "w",
Error trace : ShapeFunctions@global

We should do our own bounds checking and provide a clear and succinct message if there is a problem. I would suggest enforcing that the `"xpoints"` and `"ypoints"` arrays are the same length, and removing the `"numpoints"` field altogether as it can be inferred from `"xpoints"` and `"ypoints"`.

You were just complaining about My error messages? Not sure I should be idiot-proofing code. Now returns a friendlier OoBE.

Another thought, incompatible with the above, is that users would probably find it more intuitive to supply an array of coordinate pairs rather than a pair of coordinate arrays. E.g., allow one of these forms:

Sensible, but that's not how Java does it, and this is just exposing Java's shape constructors to macro coders. If someone wants to make variants that follow common sense, they will have to do that themselves.

[bubblobill]

Also some implementation notes. I see you've added support for ExtendedGeneralPath and even extended it to ExtendedGeneralPath_Double. But these are handled very inconsistently. If I make a "path" shape, it follows this lifecycle:

1. It is born as an ExtendedGeneralPath (single precision)
2. It is serialized to PathShapeDto to be sent to clients (including current client). But this serialization "flattens" the arc segments into cubics, so we never write out an ArcToSegment.
3. On the receiving clients, that PathShapeDto is initially deserialized to an ExtendedGeneralPath_Double (double precision). So we've "gained" extra precision.
4. Immediately that ExtendedGeneralPath_Double is converted to a Path2D.Double which has no arc support. Luckily we lost our arcs in (2) so it's not a problem!

At the end of it all, it looks like campaigns don't store any ExtendedGeneralPath or ExtendedGeneralPath_Double, just the arc-less Path2D.Double.

I actually think that outcome is for the best, at least for now. I'm a little sketched by the idea of serializaing a 3rd party type into our campaign XML, especially for the sake of an experimental feature. Once it's in there we're stuck with it. So I would suggest we explicitly avoid that, and also represent paths more consistently. I.e., use ExtendedGeneralPath when first creating the path so we get arc support, but then immediately reduce that to a plain Path2D.Double. And then have ShapeDrawable only support Path2D.Double.

Having trouble with this.

1. GeneralPath and ExtendedGeneralPath are float precision which is why I made the double version. converting to and from floats and doubles was annoying me and everything else is in double. I went to all this effort for ArcTo because nothing else actually does proper circles. Have you seen the ridiculous mathematics involved in computeArc()?
2. PathShapeDto map(PathIterator it) uses the ExtendedPathIterator.ARCTO to build the dto. At least, that what I thought I did.
3. Which is why this, because it needs something that can deal with the ArcTo segment.
4. Path2D is the superclass of GP and EGP, not sure I'm actually losing anything. If so, I want it back, whilst still being able to do path operations between different shapes.