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
Shader API improvements #561
Conversation
Codecov Report
@@ Coverage Diff @@
## master #561 +/- ##
==========================================
+ Coverage 88.88% 89.04% +0.16%
==========================================
Files 55 54 -1
Lines 11367 11515 +148
Branches 1120 1129 +9
==========================================
+ Hits 10103 10253 +150
+ Misses 965 962 -3
- Partials 299 300 +1
|
Hi @guaje, Why this PR is a draft? Do you plan to add more stuff in here? |
Yes, I'm planning to move the |
…r_code from fury.shaders.
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.
Hi @guaje,
Thank you for this update. See below for my comments
fury/shaders/base.py
Outdated
"geometry": Shader.Geometry, | ||
"fragment": Shader.Fragment, | ||
} | ||
from os.path import join as pjoin, dirname, isfile |
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.
pep8: import order. see https://peps.python.org/pep-0008/#imports for more information
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.
This should be fixed, but PTAL.
def generate_points(): | ||
centers = np.array([[1, 0, 0], [0, 1, 0], [0, 0, 1]]) | ||
|
||
scene = window.Scene() | ||
scene.add(cube) | ||
if interactive: | ||
scene.add(actor.axes()) | ||
window.show(scene) | ||
colors = np.array([[1, 0, 0], [0, 1, 0], [0, 0, 1]]) * 255 | ||
|
||
arr = window.snapshot(scene) | ||
report = window.analyze_snapshot(arr) | ||
npt.assert_equal(report.objects, 1) | ||
vtk_vertices = Points() | ||
# Create the topology of the point (a vertex) | ||
vtk_faces = CellArray() | ||
# Add points | ||
for i in range(len(centers)): | ||
p = centers[i] | ||
id = vtk_vertices.InsertNextPoint(p) | ||
vtk_faces.InsertNextCell(1) | ||
vtk_faces.InsertCellPoint(id) | ||
# Create a polydata object | ||
polydata = PolyData() | ||
# Set the vertices and faces we created as the geometry and topology of the | ||
# polydata | ||
polydata.SetPoints(vtk_vertices) | ||
polydata.SetVerts(vtk_faces) | ||
|
||
# test errors | ||
npt.assert_raises(ValueError, shader_to_actor, cube, "error", | ||
vertex_impl) | ||
npt.assert_raises(ValueError, shader_to_actor, cube, "vertex", | ||
vertex_impl, block="error") | ||
npt.assert_raises(ValueError, replace_shader_in_actor, cube, "error", | ||
vertex_impl) | ||
set_polydata_colors(polydata, colors) | ||
|
||
mapper = PolyDataMapper() | ||
mapper.SetInputData(polydata) | ||
mapper.SetVBOShiftScaleMethod(False) | ||
|
||
point_actor = Actor() | ||
point_actor.SetMapper(mapper) | ||
|
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.
Why restart from scratch and not use a point actor?
To reduce the maintenance, you should try to reduce as much as possible code duplication
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.
Hi @guaje,
thank you for the update. merging
This PR includes the following improvements: