PolylineArrowMaterial produces invalid nested structure

[zgpky]

In CZML spec, PolylineArrowMaterial should be nested as:

"material": {
  "polylineArrow": {
    "color": {...}
  }
}

However, czml3 currently outputs:

"material": {
  "polylineArrow": {
    "polylineArrow": {
      "color": {...}
    }
  }
}

Expected behavior: Remove the redundant polylineArrow level.

[Stoops-ML]

Would you like to submit a PR on this issue? And do you think you’ll be opening other issues before a patch is released?

[Stoops-ML]

I've just looked over the tests and found test_packet_polyline_arrow and test_arrowmaterial_color that test the PolylineArrowMaterial behaviour.

The tests show that there isn't the nested behaviour that you've described. Can you please provide a minimal working example to reproduce redundant polylineArrow level.

[zgpky]

Thanks for pointing that out!
You're absolutely right — my mistake was wrapping PolylineArrowMaterial inside PolylineMaterial.

So the correct usage is:

material=PolylineArrowMaterial(
    polylineArrow=PolylineArrow(color=Color(rgba=...))
)

Whereas I mistakenly did:

material=PolylineMaterial(
    polylineArrow=PolylineArrowMaterial(
        polylineArrow=PolylineArrow(color=Color(rgba=...))
    )
)

Sorry for the confusion, and thanks for clarifying with the tests!
Everything works fine now. Closing this issue.

[Stoops-ML]

Thanks for providing some code. I'm reopening this issue because it seems to me that the PolylineMaterial is unnecessary and causing wrong behaviour. I'll take a deeper look into this issue.

[Stoops-ML]

It seems that PolylineMaterial is causing wrong behaviour. The only instance of it in the tests is to define a solid colour via SolidColorMaterial; for example, test_packet_polyline.

It seems that SolidColorMaterial doesn't behave like the other materials. It should behave like PolylineArrowMaterial as you wrote above. Something like:

material=SolidColorMaterial(
    solidColor=SolidColor(color=Color(rgba=...))
)

Therefore, the following tasks are required:

• Remove PolylineMaterial
• Remove tests that specifically test PolylineMaterial
• Modify tests that call PolylineMaterial
• Modify SolidColorMaterial to behave like the other material classes, as seen above. This includes implementing a new SolidColor class

Thanks for pointing out this bug! Would you be interested in submitting a PR?

[zgpky]

Thanks for the clarification!

I also use the AGI CesiumLanguageWriter (Java CZML Writer),
and that library always sets the polyline material through a PolylineMaterialCesiumWriter.

try (PolylineMaterialCesiumWriter polylineMaterial = polyline.openMaterialProperty()) {
    try (SolidColorMaterialCesiumWriter solidColorMaterial = polylineMaterial.openSolidColorProperty()) {
        solidColorMaterial.writeColorProperty(color);
    }
}

try (PolylineMaterialCesiumWriter polylineMaterial = polyline.openMaterialProperty()) {
    try (PolylineArrowMaterialCesiumWriter polylineArrowMaterial = polylineMaterial.openPolylineArrowProperty()) {
        polylineArrowMaterial.writeColorProperty(color);
    }
}

That design influenced my implementation, which led to the problem I reported.
In other words, this issue occurred because the color could not be set directly to the PolylineArrowMaterial.

For czml3, are you planning to remove PolylineMaterial,
or would you prefer to make it behave like the Java writer for API consistency?

[Stoops-ML]

You're right about following the CZML writer, which we do indeed follow. We can see that PolylineMaterial does match the writer. My above suggestions would move away from following the CZML writer, which is not a good approach. Thanks for pointing this out!

So going back to the original problem, your code (which follows the CZML writer) is as follows:

material=PolylineMaterial(
    polylineArrow=PolylineArrowMaterial(
        polylineArrow=PolylineArrow(color=Color(rgba=...))
    )
)

This produces:

"material": {
  "polylineArrow": {
    "polylineArrow": {
      "color": {...}
    }
  }
}

The bug here is that there is an additional polylineArrow level that needs to be removed. The output should be:

"material": {
  "polylineArrow": {
    "color": {...}
  }
}

The problem in the code is that Polyline.material currently accepts:

• None
• PolylineMaterial
• PolylineDashMaterial
• PolylineArrowMaterial
• PolylineGlowMaterial
• PolylineOutlineMaterial
• str
• TimeIntervalCollection

According to the polyline CZML writer the material property should only accept PolylineMaterial. Hence, Polyline.material should accept:

• None
• PolylineMaterial
• str
• TimeIntervalCollection

Note that Polyline.depthFailMaterial has the same bug here.

The second problem seems to be that some properties in PolylineMaterial point to another property that points to another property (i.e. there's too much nesting, which is what we see in the output). The following properties in PolylineMaterial are the offending classes:

• PolylineDashMaterial
• PolylineOutlineMaterial
• PolylineArrowMaterial
• PolylineGlowMaterial

So the breakdown of tasks to fix this bug is:

• All Polyline materials (i.e. Polyline.material and Polyline.depthFailMaterial) need to be correctly typed
• Remove the stacking from the following properties: PolylineDashMaterial, PolylineOutlineMaterial, PolylineArrowMaterial and PolylineGlowMaterial. For example, the properties in PolylineDash need to moved to PolylineDashMaterial and PolylineDash needs to be deleted
• Check all other materials in the code to ensure that this problem isn't repeated
• Modify tests that are impacted by the above changes, including removing irrelevant tests. Note that we require 99.5% coverage

Would you be interested in submitting a PR?

[zgpky]

Thanks!
I’ll open a PR based on your breakdown.

[Stoops-ML]

Thanks very much!!

Remember to keep your commits small and commit messages concise yet informative.

You can always submit PRs if you have any questions. Good luck!

[zgpky]

I’ve opened PR #64 to address this issue.

[Stoops-ML]

#64 has been merged! Thank you!

I'll release v3.0.0 in the near future. This will give time for any other code changes.