czml3 icon indicating copy to clipboard operation
czml3 copied to clipboard

Add _repr_svg_()

Open Stoops-ML opened this issue 1 year ago • 11 comments

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:

  • Polyline()
  • Polygon()
  • Position()
  • PositionList()
  • Packet()
  • Wall()
  • Corridor()

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 avatar May 20 '23 21:05 Stoops-ML

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

Stoops-ML avatar May 20 '23 21:05 Stoops-ML

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

astrojuanlu avatar May 20 '23 21:05 astrojuanlu

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: @.***>

Stoops-ML avatar May 20 '23 22:05 Stoops-ML

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
@@            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 Coverage Δ
src/czml3/types.py 98.02% <ø> (ø)
src/czml3/properties.py 93.92% <72.17%> (-6.08%) :arrow_down:
src/czml3/core.py 91.20% <83.33%> (-8.80%) :arrow_down:
src/czml3/base.py 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.

codecov-commenter avatar May 21 '23 06:05 codecov-commenter

This patch fixes:

  • linting
  • Packet() creates SVG of Point() and Path() instead of Position() and PositionList() respectively

Stoops-ML avatar May 21 '23 06:05 Stoops-ML

This push increases code coverage.

Stoops-ML avatar May 21 '23 07:05 Stoops-ML

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 avatar May 21 '23 07:05 Stoops-ML

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

Also, any tips or feedback would be greatly appreciated!

Stoops-ML avatar May 21 '23 10:05 Stoops-ML

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:

  • Find a good moment to review it. It won't happen soon, and by the time I get to it (if I ever can), you will be frustrated or not interested anymore.
  • Merge without reviewing, trusting that the tests are enough. I think it would be a disservice to me as maintainer and to czml3 users.

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

  • It will bring interesting use cases beyond SVG representation, and
  • It will be much less code, hence easier to review.

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

astrojuanlu avatar May 21 '23 15:05 astrojuanlu

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 avatar May 21 '23 17:05 Stoops-ML

95.24% coverage.

Stoops-ML avatar May 22 '23 06:05 Stoops-ML