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

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

Merged
merged 26 commits into from
Jul 29, 2023

Conversation

ganimtron-10
Copy link
Contributor

@ganimtron-10 ganimtron-10 commented Jun 15, 2023

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.

@ganimtron-10 ganimtron-10 marked this pull request as ready for review July 5, 2023 18:47
@ganimtron-10 ganimtron-10 changed the title [WIP] Adding Bounding Box to TextBlock2D Adding Bounding Box & Fixing Alignment issue in TextBlock2D Jul 5, 2023
@skoudoro
Copy link
Contributor

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
Copy link
Contributor Author

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

@ganimtron-10 ganimtron-10 changed the title Adding Bounding Box & Fixing Alignment issue in TextBlock2D UI: Adding Bounding Box & Fixing Alignment issue in TextBlock2D Jul 21, 2023
@JoaoDell
Copy link
Contributor

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
Copy link
Contributor Author

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!

Copy link
Contributor

@JoaoDell JoaoDell left a comment

Choose a reason for hiding this comment

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

Hey @ganimtron-10, I have just finished my first review here and, overall, it seems fine and working. I have ran the ui test and everything passed, so it does not seem to have any problem. I would just like to know if you recommend any example to be ran so I can check the new features and changes visually. Nice work!

fury/ui/core.py Outdated Show resolved Hide resolved
@ganimtron-10
Copy link
Contributor Author

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()

Copy link
Contributor

@tvcastillod tvcastillod left a comment

Choose a reason for hiding this comment

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

Hi @ganimtron-10, I think it is looking good, you have made great progress. I left a few comments and questions below for you to check them. And I have another question related to the two enhancements mentioned.

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_font_scale_, the bounding box is the one which set the size of the text, but with dynamic_bbox, it is the text size that sets the bounding box size. Did I understand correctly?

fury/ui/core.py Outdated Show resolved Hide resolved
fury/ui/core.py Show resolved Hide resolved
fury/ui/core.py Outdated Show resolved Hide resolved
fury/ui/core.py Show resolved Hide resolved
@ganimtron-10
Copy link
Contributor Author

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_font_scale_, the bounding box is the one which set the size of the text, but with dynamic_bbox, 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
Copy link
Contributor Author

Thanks @JoaoDell @tvcastillod for the review!

@skoudoro
Copy link
Contributor

Hi @ganimtron-10,

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

Copy link
Contributor

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

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

Overall, it looks good.

The code is better like that, it is much simpler. I still need to play a bit more.
see below few comments that can be fixed quickly

Thanks @ganimtron-10

fury/ui/core.py Outdated Show resolved Hide resolved
fury/ui/core.py Outdated Show resolved Hide resolved
fury/ui/core.py Outdated Show resolved Hide resolved
@ganimtron-10
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link

codecov bot commented Jul 28, 2023

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%) ⬇️
fury/ui/core.py 93.22% <95.58%> (-0.17%) ⬇️
fury/ui/helpers.py 96.10% <100.00%> (-0.24%) ⬇️

... and 7 files with indirect coverage changes

@ganimtron-10
Copy link
Contributor Author

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

@skoudoro
Copy link
Contributor

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

Copy link
Contributor

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

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

Nice work @ganimtron-10! Let's move forward. merging

@skoudoro skoudoro merged commit 924d024 into fury-gl:master Jul 29, 2023
19 of 30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants