GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — add `size_space` to `pygfx.PointsMaterial` scatter kwargs by FlynnOConnell · Pull Request #689 · fastplotlib/fastplotlib

Conversation

@FlynnOConnell

@FlynnOConnell FlynnOConnell commented Jan 7, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

The size_space property of PointsMaterial controls which coordinate space to use for the sizing of each point. Default is screen, which using logical pixel values mapped to the screen. Zooming into a plot causes the thickness of circles to remain the same thickness. see here

This aims to allow the other options of CoordSpace Enum to be used instead (world, model, screen).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@FlynnOConnell
add size_space to pygfx.PointsMaterial kwargs
f64ff43
@FlynnOConnell

FlynnOConnell commented Jan 7, 2025

Copy link
Copy Markdown
Collaborator Author

I'm not immediately seeing the need for a GraphicFeature here, but I could be wrong?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@kushalkolar

kushalkolar commented Jan 7, 2025

Copy link
Copy Markdown
Member

All aspects of a graphic are managed by a GraphicFeature class and exposed to the user as properties: https://www.fastplotlib.org/ver/dev/developer_notes/graphics.html#graphic-properties

Lines also have a thickness_space parameter so this feature can actually be settable with a property of the PositionsGraphic base class.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

FlynnOConnell added 4 commits January 7, 2025 16:26
@FlynnOConnell
add CoordSpace graphic feature
7edc007
@FlynnOConnell
store _coord_space graphic feature
83f3c17
@FlynnOConnell
add coordspace to features init
113ff68
@FlynnOConnell
make size_space a property of PositionsGraphic
e54b928
kushalkolar
kushalkolar requested changes Jan 7, 2025
View reviewed changes

@kushalkolar kushalkolar left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A good start!

This doesn't need to be specific to scatters, pygfx line material also uses a thickness_space property that takes the same args and does an analogous thing so this can be in the PositionsGraphic base class.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
FlynnOConnell added 4 commits January 7, 2025 17:17
@FlynnOConnell
rename CoordSpace->SizeSpace for consistency / avoid confusion with p…
77f4c40
…ygfx.CoordSpace
@FlynnOConnell
fix size_space feature, FeatureEvent type=size_space??
9ff2abc
@FlynnOConnell
add getters/setters to scatter/line graphics
b398b72
@FlynnOConnell
add size_space to _features cls dict
3e527cc
kushalkolar
kushalkolar requested changes Jan 7, 2025
View reviewed changes

@kushalkolar kushalkolar left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will also need a simple test when you're done, see this for an outline:

fastplotlib/tests/test_positions_graphics.py

Lines 419 to 445 in f204f55

@pytest.mark.parametrize("thickness", [None, 0.5, 5.0])
def test_thickness(thickness):
fig = fpl.Figure()
kwargs = dict()
for kwarg in ["thickness"]:
if locals()[kwarg] is not None:
# add to dict of arguments that will be passed
kwargs[kwarg] = locals()[kwarg]
data = generate_positions_spiral_data("xy")
graphic = fig[0, 0].add_line(data=data, **kwargs)
if thickness is None:
thickness = 2.0 # default thickness
assert isinstance(graphic._thickness, Thickness)
assert graphic.thickness == thickness
assert graphic.world_object.material.thickness == thickness
if thickness == 0.5:
assert isinstance(graphic.world_object.material, pygfx.LineThinMaterial)
else:
assert isinstance(graphic.world_object.material, pygfx.LineMaterial)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread fastplotlib/graphics/scatter.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread fastplotlib/graphics/scatter.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread fastplotlib/graphics/line.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread fastplotlib/graphics/line.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

FlynnOConnell added 2 commits January 7, 2025 18:27
@FlynnOConnell
move size_space properties to base PositionGraphics class, add link t…
7e88a08
…o CoordSpace in docstring
@FlynnOConnell
fix comment for SizeSpace scatter/line
b8a9be3
kushalkolar
kushalkolar requested changes Jan 8, 2025
View reviewed changes
Comment thread fastplotlib/graphics/_features/_positions_graphics.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread fastplotlib/graphics/_positions_base.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread fastplotlib/graphics/_positions_base.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

FlynnOConnell added 2 commits January 9, 2025 10:21
@FlynnOConnell
return .value, set_value()
8a0b9be
@FlynnOConnell
don't coerce str, check if "Line" in class name
5db629b
@FlynnOConnell

FlynnOConnell commented Jan 9, 2025

Copy link
Copy Markdown
Collaborator Author

Eye test looks like everything checks out. will work on a test case next

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@FlynnOConnell
add test for size_space
07de27b
@FlynnOConnell
FlynnOConnell marked this pull request as ready for review January 9, 2025 16:43
@FlynnOConnell
FlynnOConnell requested a review from clewis7 as a code owner January 9, 2025 16:43
kushalkolar
kushalkolar requested changes Jan 9, 2025
View reviewed changes
Comment thread tests/test_positions_graphics.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@FlynnOConnell
add tests for setting properties
68c83eb
kushalkolar
kushalkolar requested changes Jan 9, 2025
View reviewed changes
Comment thread tests/test_positions_graphics.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@FlynnOConnell
double quotes for the linting overlords
000d6a3
@kushalkolar

kushalkolar commented Jan 9, 2025

Copy link
Copy Markdown
Member

Since you've added/changed the API you will need to regenerate the API docs:

in docs/source:

delete the api dir

with the env activated, run:

python generate_api.py

add and commit docs/api

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@FlynnOConnell
regen api docs
613ece7
@kushalkolar

kushalkolar commented Jan 9, 2025

Copy link
Copy Markdown
Member

black is still unhappy, just do black fastplotlib/

only black format the fastplotlib source dir, not the examples or tests etc.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@FlynnOConnell
double quotes on "size_space"
38c1897
@FlynnOConnell
black formatting spaces / trailing commas
18b3455
@FlynnOConnell

FlynnOConnell commented Jan 9, 2025

Copy link
Copy Markdown
Collaborator Author

plz tell black to stop complaining

image

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@FlynnOConnell

FlynnOConnell commented Jan 9, 2025

Copy link
Copy Markdown
Collaborator Author

@kushalkolar I don't even have a guess for why the docs failed

Run SamKirkland/[email protected] Error: Error: Input required and not supplied: server

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@kushalkolar

kushalkolar commented Jan 9, 2025

Copy link
Copy Markdown
Member

I think it's trying to access the credentials which are repo secrets and it won't since you're on a fork, we'll figure out later how to make docs upload from forks. Anyways the docs built without issues so that's fine and everything else LGTM.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

kushalkolar
kushalkolar approved these changes Jan 9, 2025
View reviewed changes
@kushalkolar
kushalkolar merged commit 962c9ba into fastplotlib:main Jan 9, 2025
@kushalkolar kushalkolar mentioned this pull request Feb 20, 2025
replace weird quotes, update GraphicMethodsMixin #735
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kushalkolar kushalkolar kushalkolar approved these changes
@clewis7 clewis7 Awaiting requested review from clewis7 clewis7 is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@FlynnOConnell @kushalkolar
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.