fury-gl / fury

FURY - Free Unified Rendering in pYthon.
https://fury.gl
Other
241 stars 181 forks source link

UI: Adding Bounding Box & Fixing Alignment issue in TextBlock2D #803

Closed ganimtron-10 closed 1 year ago

ganimtron-10 commented 1 year ago

The TextBlock2D component in the UI system was encountering alignment and scaling issues. When resizing the TextBlock2D, the text scaling mode was set to "SetTextScaleModeToProp," which resulted in inconsistent alignment of the text actor and improper justification of the text with respect to the background rectangle. Additionally, the background rectangle's size was calculated only after it was added to the scene, which is not an ideal practice.

To address these issues, this PR introduces the following enhancements:

  1. BoundingBox Property: A new property called "boundingbox" is implemented in the TextBlock2Dcomponent. This property calculates the text bounding box based on the content of the TextBlock2D. It ensures accurate alignment and justification of the text within the component.

  2. Auto Font Scale: The scaling mode is now separated from the resizing action. A new property named "auto_font_scale" is introduced, which, when set to True, enables automatic font scaling according to the bounding box. This allows for consistent and proportional text sizing without manual intervention.

By implementing these changes, the TextBlock2D component will provide improved text alignment, justified text with the background rectangle, and a more intuitive font scaling experience.

skoudoro commented 1 year ago

Hi @ganimtron-10, What is the status of this PR?

Many tests are failing! I suppose that you might need to record again many tests because of this change. Before doing that, let me know when I can check and test this PR

ganimtron-10 commented 1 year ago

Sure @skoudoro will let you know, Currently working on the tests itself.

JoaoDell commented 1 year ago

Hey @ganimtron-10 is this ready for review or should we wait for you to ping us again? I see that you made some commits yesterday, so I don't know whether I should start or wait.

ganimtron-10 commented 1 year ago

Hey @JoaoDell you can go ahead! I was just waiting for the tests to get completed so that I could make sure the changes doesn't hamper anything! As the tests are complete now you can go ahead!

ganimtron-10 commented 1 year ago

Hey @JoaoDell , You can try out this below code to try out the feature visually.

from fury import ui, window
from fury.data import fetch_viz_icons

fetch_viz_icons()

c = ui.TextBox2D(width=12, height=3, justification='center') #left, right, center

show_manager = window.ShowManager(size=(500, 500),
                                  title="FURY textbox Example")

show_manager.scene.add(c)

show_manager.start()
ganimtron-10 commented 1 year ago

Between auto_font_scale and dynamic_bbox, which one has the priority (what happens when I set both true)? because both seem to be opposite as with_auto_fontscale, the bounding box is the one which set the size of the text, but with _dynamicbbox, it is the text size that sets the bounding box size. Did I understand correctly?

Both of the parameter can co exist! See auto_font_scale parameter helps to scale the font according to the background boundingbox, whereas dynamic_bbox helps to dynamically modify the boundingbox according to the text not according to the text font. So when both is applied the background box is dynamic and the text font is scaled accordingly!

ganimtron-10 commented 1 year ago

Thanks @JoaoDell @tvcastillod for the review!

skoudoro commented 1 year ago

Hi @ganimtron-10,

Tests are still failing, do you plan to look at them ? please, look at tabui

ganimtron-10 commented 1 year ago

Hey @skoudoro ,

Tests are still failing, do you plan to look at them ? please, look at tabui

I checked those last time and it worked fine! Maybe I saw outdated version, My bad. Also it is a bit weird to me that in the tabui test, we are equating title_font_size to 12 where as the default size is 18. Am I missing some thing? I checked out the code are we are not manually setting the font_size too.

ganimtron-10 commented 1 year ago

I was also wondering do we need wrap_overflow helper function, as the functionality is now directly available in the TextBlock2D as dynamic_bbox. I searched the whole codebase and we don't directly use it anywhere.

codecov[bot] commented 1 year ago

Codecov Report

Merging #803 (0ecdc84) into master (73e0a1a) will decrease coverage by 0.12%. Report is 148 commits behind head on master. The diff coverage is 95.83%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #803      +/-   ##
==========================================
- Coverage   84.41%   84.30%   -0.12%     
==========================================
  Files          43       44       +1     
  Lines       10166    10353     +187     
  Branches     1381     1406      +25     
==========================================
+ Hits         8582     8728     +146     
- Misses       1227     1255      +28     
- Partials      357      370      +13     
Files Changed Coverage Δ
fury/ui/elements.py 89.78% <ø> (-0.14%) :arrow_down:
fury/ui/core.py 93.22% <95.58%> (-0.17%) :arrow_down:
fury/ui/helpers.py 96.10% <100.00%> (-0.24%) :arrow_down:

... and 7 files with indirect coverage changes

ganimtron-10 commented 1 year ago

Hey @skoudoro , I have made most of the necessary changes and the test are also working fine!

skoudoro commented 1 year ago

Great! let me play a bit with this and we can move forward