Enh textblock #1385
Enh textblock #1385
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1385 +/- ##
==========================================
+ Coverage 87.41% 87.42% +<.01%
==========================================
Files 238 238
Lines 30282 30401 +119
Branches 3253 3274 +21
==========================================
+ Hits 26472 26577 +105
- Misses 3057 3065 +8
- Partials 753 759 +6
Continue to review full report at Codecov.
|
|
So, what's happening with this PR? The tests are passing. I don't think there is much I can do about the coverage, though. |
dipy/viz/tests/test_ui.py
Outdated
| show_manager.ren.add(*texts) | ||
|
|
||
| # Uncomment this to start the visualisation | ||
| show_manager.start() |
There was a problem hiding this comment.
It's not necessary to start before taking the snapshot, and this halts the test, waiting for the user to close the window.
There was a problem hiding this comment.
Right, it was left over for my debugging.
dipy/viz/tests/test_ui.py
Outdated
| arr = window.snapshot(show_manager.ren, size=window_size, offscreen=True) | ||
| if vtk.vtkVersion.GetVTKMajorVersion() >= 6: | ||
| expected = np.load(pjoin(DATA_DIR, "test_ui_text_block.npz")) | ||
| npt.assert_array_equal(arr, expected["arr_0"]) |
There was a problem hiding this comment.
On Windows 10, Python 3.6, VTK 7.1 this assertion fails:
AssertionError:
Arrays are not equal
(mismatch 3.919795918367342%)
x: array([[[0, 0, 0],
[0, 0, 0],
[0, 0, 0],...
y: array([[[0, 0, 0],
[0, 0, 0],
[0, 0, 0],...
There was a problem hiding this comment.
@Garyfallidis suggests using assert_array_almost_equal here. It may be reasonable for two GPUs to differ by 4% when generating these images.
There was a problem hiding this comment.
Sounds good. I'll push that modification but can't easily test it. Can you confirm it is now passing on Wind10, Vtk7.1 ?
| text_property.SetVerticalJustificationToTop() | ||
| else: | ||
| msg = "Vertical justification must be: bottom, middle or top." | ||
| raise ValueError(msg) |
There was a problem hiding this comment.
Can we test that this error is raised?
|
@MarcCote any thoughts on my review? |
|
@dmreagan I think I've addressed all your comments, let me know if there's anything else. |
|
Everything looks good to me, but it seems we'll have to wait for a resolution to #1435 before merging. |
|
can you rebase @MarcCote? It seems to be ready to go. Thanks! |
|
I've already rebased :) |
|
my bad, thanks ! merging ! |
Enh textblock
Adds support for vertical alignment of text in a TextBlock.
Adds a background to TextBlock objects. I made/hack a version for VTK < 7 but the behavior is somewhat different than using vtkTextActor.GetTextProperty().SetBackgroundColor(). I don't know what we should do in this case. I guess it will depend on which versions of VTK we want to support.
I added a unit test to make sure the text alignment are working properly (no surprise they are not in vtk 5.8). The test consists in comparing a snapshot to a static version I've included in this PR (works with VTK 6). Any suggestions to improve it is welcome.