poliastro / czml3

Python 3 library to write CZML
https://pypi.org/project/czml3/
MIT License
37 stars 30 forks source link

Add _repr_svg_() #111

Open Stoops-ML opened 1 year ago

Stoops-ML commented 1 year ago

An SVG image is displayed for BaseCZMLObject() objects that have Position() or PositionList() attributes (if defined). The Colour() attribute is used if defined, else black is used.

No image is displayed for BaseCZMLObject() objects that don't have Position() or PositionList() attributes, nor for BaseCZMLObject() objects that don't have Position() or PositionList() attributes defined.

This is supported for:

Note that the Packet() class combines all SVGs that it contains, and supports Point() and Path() classes which have their positions defined externally (i.e. within the packet and not within themselves).

An example in Jupyter lab:

p = Packet(
        id="AreaTarget/Pennsylvania",
        name="Pennsylvania",
        position=Position(
            cartographicDegrees=[38, 38, 10]
        ),
        point=Point(
            show=True,
            pixelSize=10,
            scaleByDistance=NearFarScalar(
                nearFarScalar=NearFarScalarValue(values=[150, 2.0, 15000000, 0.5])
            ),
            disableDepthTestDistance=1.2,
            color=Color(rgba=[200, 100, 30, 255]),
        ),
        polygon=Polygon(
positions=PositionList(
    cartographicDegrees=CartographicDegreesListValue(values=[37.2, 37.3, 10, 38, 38, 10, 37.5, 36.9, 10])
),
material=Material(solidColor=SolidColorMaterial.from_list([200, 100, 30])),
),
polyline=Polyline(
    material=Material(solidColor=SolidColorMaterial.from_list([0, 255, 0])),
    positions=PositionList(
        cartographicDegrees=CartographicDegreesListValue(values=[37, 37, 10, 36, 36, 10])
    ),
    arcType=ArcType(arcType="GEODESIC"),
    distanceDisplayCondition=DistanceDisplayCondition(
        distanceDisplayCondition=DistanceDisplayConditionValue(values=[14, 81])
    ),
    classificationType=ClassificationType(
        classificationType=ClassificationTypes.CESIUM_3D_TILE
    ),
)
    )
p

image

Stoops-ML commented 1 year ago

Quality checks have failed. I'll fix this and do another PR.

astrojuanlu commented 1 year ago

No need to do another PR! You can keep pushing commits to the same branch until the CI goes green

Stoops-ML commented 1 year ago

Thanks for letting me know. Still getting to grips with this!

On Sun, 21 May 2023, 00:57 Juan Luis Cano Rodríguez, < @.***> wrote:

No need to do another PR! You can keep pushing commits to the same branch until the CI goes green

— Reply to this email directly, view it on GitHub https://github.com/poliastro/czml3/pull/111#issuecomment-1556023677, or unsubscribe https://github.com/notifications/unsubscribe-auth/AO4AFMC2E4WWHFYGWDJQKPDXHE46JANCNFSM6AAAAAAYI6UBME . You are receiving this because you modified the open/close state.Message ID: @.***>

codecov-commenter commented 1 year ago

Codecov Report

Patch coverage: 79.48% and project coverage change: -3.99 :warning:

Comparison is base (feb7f0a) 99.22% compared to head (c5968a7) 95.24%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #111 +/- ## ========================================== - Coverage 99.22% 95.24% -3.99% ========================================== Files 12 12 Lines 773 967 +194 ========================================== + Hits 767 921 +154 - Misses 6 46 +40 ``` | [Impacted Files](https://app.codecov.io/gh/poliastro/czml3/pull/111?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=poliastro) | Coverage Δ | | |---|---|---| | [src/czml3/types.py](https://app.codecov.io/gh/poliastro/czml3/pull/111?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=poliastro#diff-c3JjL2N6bWwzL3R5cGVzLnB5) | `98.02% <ø> (ø)` | | | [src/czml3/properties.py](https://app.codecov.io/gh/poliastro/czml3/pull/111?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=poliastro#diff-c3JjL2N6bWwzL3Byb3BlcnRpZXMucHk=) | `93.92% <72.17%> (-6.08%)` | :arrow_down: | | [src/czml3/core.py](https://app.codecov.io/gh/poliastro/czml3/pull/111?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=poliastro#diff-c3JjL2N6bWwzL2NvcmUucHk=) | `91.20% <83.33%> (-8.80%)` | :arrow_down: | | [src/czml3/base.py](https://app.codecov.io/gh/poliastro/czml3/pull/111?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=poliastro#diff-c3JjL2N6bWwzL2Jhc2UucHk=) | `97.10% <100.00%> (+2.50%)` | :arrow_up: |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

Stoops-ML commented 1 year ago

This patch fixes:

Stoops-ML commented 1 year ago

This push increases code coverage.

Stoops-ML commented 1 year ago

What do you mean exactly by canvas size and coordinates mangling? Sorry for my ignorance, I'm new to this so please be patient with me!

The approach I followed is based on what Shapely does. See here for how they make a point, and here for how they combine several points (or anything else) together.

To clarify so that we're on the same page: the canvas size depends on where the points on the canvas are. The closer the points the smaller the canvas size desired (as long as it's bigger than the minimum canvas size). This results in closer points on a canvas looking bigger on the screen, and farther away points on a canvas looking smaller.

The main issue I have with calculating relative coordinates is that it will add a lot of complexity to the code in the Packet() class that will replace the simple bounds calculation. And after all that work the created SVG would be the same between the two implementations.

I'm not sure what mangling exactly means so I may be wrong.

Stoops-ML commented 1 year ago

I've done some refactoring, so hopefully things are a bit more clear now.

Also, any tips or feedback would be greatly appreciated!

astrojuanlu commented 1 year ago

I see! If Shapely already has this logic, wouldn't it be easier if we

  1. Transform czml3 entities into Shapely geometries when appropriate, then
  2. Use Shapely SVG logic to show those?

That way, we would avoid having to maintain lots of code, and benefit from any improvements done in Shapely.

In fact, this can be more widely useful: if we were able to transform czml3 entities to GeoJSON, I bet many people would benefit from that (see for example gh-83). Then the conversion would be czml3 -> GeoJSON -> Shapely -> SVG.

I appreciate this contribution a lot and I'm sure you put lots of hours into it, which I'm grateful for. However, I think we should rework it. In its current form, I have two options:

On the other hand, if we find another way of achieving the same by leveraging the Python ecosystem:

Hope this doesn't come across as blunt or ungrateful!

Stoops-ML commented 1 year ago

I personally don't like the idea of using Shapely for the following reasons:

  1. The logic of creating the SVG image is rather simple: get the coordinates, get the bounds, get the colour if applicable, and create the frame.
  2. Making Shapely a requirement for using czml3 requires taking on all of Shapely's package requirements and Python version limitations - just for the sake of outputting SVGs in Jupyter notebook/lab. In my opinion that is a bloating of the user's environment.

Regarding your point of leveraging the Python ecosystem, i.e. Shapely:

  1. I don't think using Shapely will reduce the amount of code to output the SVG.
  2. I'm not convinced that leveraging Shapely to reduce the amount of code that needs to be maintained is correct. I can see it going both ways: perhaps we can rely on Shapely to fix all bugs related to SVGs etc, and/or perhaps Shapely changes their code requiring us to change ours (most likely without any warning).
  3. In reference to use cases beyond SVG in Shapely, I don't think czml3 should become a wrapper for Shapely. People who use czml3 and Shapely together should use them individually as both libraries require very similar inputs from the user (i.e. just coordinates). Perhaps I'm missing the interesting use cases of connecting the two.

Finally, I understand that you don't have much time to review code. However, no matter what the PR is you'll have to review it. There's definitely a trade-off between accepting contributions and maintaining package stability and maintainability, and I'm sure you'll strike the right balance that's in the best interest of the community.

Stoops-ML commented 1 year ago

95.24% coverage.