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

Enh textblock #1385

Merged
merged 4 commits into from Mar 3, 2018

Conversation

Projects
4 participants
@MarcCote
Contributor

MarcCote commented Nov 30, 2017

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.

@codecov-io

This comment has been minimized.

codecov-io commented Nov 30, 2017

Codecov Report

Merging #1385 into master will increase coverage by <.01%.
The diff coverage is 86.95%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
dipy/viz/ui.py 89.29% <79.06%> (-0.58%) ⬇️
dipy/viz/tests/test_ui.py 85.42% <96%> (+1.88%) ⬆️
dipy/core/graph.py 75% <0%> (+1.19%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a8ed85...f646566. Read the comment docs.

@dmreagan dmreagan added this to UI PR needs a review in Viz Module Dec 1, 2017

@dmreagan

This comment has been minimized.

Contributor

dmreagan commented Dec 1, 2017

@MarcCote I just realized we duplicated some effort by adding background color to TextBlock2d. See #1362. I haven't looked closely enough yet to offer a solution. First to get merged wins!

@MarcCote MarcCote referenced this pull request Dec 12, 2017

Merged

WIP: ENH: UI Listbox #1355

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Jan 19, 2018

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.

dmreagan added a commit to dmreagan/dipy that referenced this pull request Jan 19, 2018

show_manager.ren.add(*texts)
# Uncomment this to start the visualisation
show_manager.start()

This comment has been minimized.

@dmreagan

dmreagan Feb 2, 2018

Contributor

It's not necessary to start before taking the snapshot, and this halts the test, waiting for the user to close the window.

This comment has been minimized.

@MarcCote

MarcCote Feb 18, 2018

Contributor

Right, it was left over for my debugging.

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"])

This comment has been minimized.

@dmreagan

dmreagan Feb 2, 2018

Contributor

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],...

This comment has been minimized.

@dmreagan

dmreagan Feb 15, 2018

Contributor

@Garyfallidis suggests using assert_array_almost_equal here. It may be reasonable for two GPUs to differ by 4% when generating these images.

This comment has been minimized.

@MarcCote

MarcCote Feb 18, 2018

Contributor

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)

This comment has been minimized.

@dmreagan

dmreagan Feb 2, 2018

Contributor

Can we test that this error is raised?

This comment has been minimized.

@MarcCote

MarcCote Feb 18, 2018

Contributor

I think I do now.

@dmreagan

This comment has been minimized.

Contributor

dmreagan commented Feb 15, 2018

@MarcCote any thoughts on my review?

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Feb 18, 2018

@dmreagan I think I've addressed all your comments, let me know if there's anything else.

@dmreagan

This comment has been minimized.

Contributor

dmreagan commented Feb 21, 2018

Everything looks good to me, but it seems we'll have to wait for a resolution to #1435 before merging.

@MarcCote MarcCote force-pushed the MarcCote:enh_textblock branch from 7fcb7de to f646566 Mar 3, 2018

@skoudoro

This comment has been minimized.

Member

skoudoro commented Mar 3, 2018

can you rebase @MarcCote? It seems to be ready to go. Thanks!

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Mar 3, 2018

I've already rebased :)

@skoudoro skoudoro merged commit 01d01e2 into nipy:master Mar 3, 2018

2 of 3 checks passed

codecov/patch 86.95% of diff hit (target 87.41%)
Details
codecov/project 87.42% (+<.01%) compared to 5a8ed85
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Viz Module automation moved this from UI PR needs a review to Done Mar 3, 2018

@skoudoro

This comment has been minimized.

Member

skoudoro commented Mar 3, 2018

my bad, thanks ! merging !

@MarcCote MarcCote deleted the MarcCote:enh_textblock branch Mar 4, 2018

ShreyasFadnavis pushed a commit to ShreyasFadnavis/dipy that referenced this pull request Sep 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment