Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compatibility with vedo 2023.5.0 #277

Merged
merged 7 commits into from
Nov 16, 2023

Conversation

IgorTatarnikov
Copy link
Member

Before submitting a pull request (PR), please read the contributing guide.

Description

What is this PR

This PR addresses any changed to the API made in vedo 2023.5.0.

  • Bug fix
  • Addition of a new feature
  • Other

Why is this PR needed?

vedo 2023.5.0 was released.

References

#261

How has this PR been tested?

All tests pass locally, all examples apart from examples/streamlines.py run.

Is this a breaking change?

If this PR breaks any existing functionality, please explain how and why.

Checklist:

  • The code has been tested locally
  • The code has been formatted with pre-commit

@IgorTatarnikov IgorTatarnikov marked this pull request as ready for review November 16, 2023 11:42
Copy link

codecov bot commented Nov 16, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (9a24de2) 67.86% compared to head (cf07e74) 72.44%.
Report is 7 commits behind head on mega-fix.

Files Patch % Lines
brainrender/actors/cylinder.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           mega-fix     #277      +/-   ##
============================================
+ Coverage     67.86%   72.44%   +4.57%     
============================================
  Files            26       26              
  Lines          1195     1194       -1     
============================================
+ Hits            811      865      +54     
+ Misses          384      329      -55     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@alessandrofelder alessandrofelder left a comment

Choose a reason for hiding this comment

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

Looks good - couple of tiny comments.

As I presume you've seen, the new way to set the color mode (?) can be gleaned from this vedo example - no idea whether we want to/need/should support this (@adamltyson ?)

maybe open a low priority issue so we don't forget? Unless you already had another plan around how to fix this, @IgorTatarnikov ?

@@ -37,7 +37,7 @@ def ruler(p1, p2, unit_scale=1, units=None, s=50):
dist = mag(p2 - p1) * unit_scale
label = precision(dist, 3) + " " + units
lbl = Text3D(label, pos=midpoint, s=s + 100, justify="center")
lbl.SetOrientation([0, 0, 180])
# lbl.SetOrientation([0, 0, 180])
Copy link
Member

Choose a reason for hiding this comment

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

Can this be removed entirely?

Copy link
Member Author

Choose a reason for hiding this comment

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

I only left it as a sort of reminder that this used to do something and now it doesn't. The labels aren't in the right place when the add_labels.py example is run so I thought that'll be a good first place to look when we get to fixing it.

@@ -13,7 +13,7 @@
scene = Scene(title="neurons")

# Add a neuron from file
scene.add(Neuron("examples/data/neuron1.swc"))
scene.add(Neuron("data/neuron1.swc"))
Copy link
Member

Choose a reason for hiding this comment

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

Do this (and similar) changes in the path imply that the examples have to be run from the examples/ subdirectory? If so, should we make an issue to generalise this using e.g. importlib.resources?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can definitely make an issue, most paths need to be cleaned up so they're not as fragile

@@ -32,7 +32,7 @@
"focalPoint": (7718, 4290, -3507),
"distance": 40610,
}
zoom = 1.5
zoom = 2.5
Copy link
Member

Choose a reason for hiding this comment

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

Should we have a quick comment here to explain this magic number? Presumably it's just something like "Make the screenshot look nicer by zooming up closer"?

Copy link
Member Author

Choose a reason for hiding this comment

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

I honestly just took it from here assuming that something changed with how the zoom is calculated in vedo.

@@ -74,7 +74,8 @@

# 3. create a Volume vedo actor and smooth
print("Creating volume")
vol = Volume(transformed_stack, origin=scene.root.origin()).smooth_median()
vol = Volume(transformed_stack).permute_axes(2, 1, 0)
Copy link
Member

Choose a reason for hiding this comment

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

Quick comment to explain why the axis permutation is needed?

Copy link
Member Author

@IgorTatarnikov IgorTatarnikov Nov 16, 2023

Choose a reason for hiding this comment

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

I'm planning to move that out of the examples and into the core package, I've left some of the post compatibility changes off of this PR.

@alessandrofelder alessandrofelder mentioned this pull request Nov 16, 2023
7 tasks
@adamltyson
Copy link
Member

As I presume you've seen, the new way to set the color mode (?) can be gleaned from this vedo example - no idea whether we want to/need/should support this (@adamltyson ?)

No idea tbh

@alessandrofelder
Copy link
Member

No idea tbh

I'd suggest leaving it out then until a user asks for the functionality, but also happy with an additional mode argument for if we wanna support it - no strong opinion. This can be merged once you're happy with it @IgorTatarnikov - I am.

@IgorTatarnikov IgorTatarnikov merged commit 02b7565 into mega-fix Nov 16, 2023
18 checks passed
@alessandrofelder alessandrofelder deleted the compatibility-with-vedo-2023.5.0+dev26a branch November 16, 2023 13:48
@alessandrofelder alessandrofelder mentioned this pull request Dec 1, 2023
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants