Skip to content
This repository was archived by the owner on Apr 18, 2024. It is now read-only.

Conversation

@Gondragos
Copy link
Contributor

PR fulfills these requirements

  • Commit message(s) and PR title follows the format [fix|feat|ci|chore|doc]: TICKET-ID: Short description of change made ex. fix: DEV-XXXX: Removed inconsistent code usage causing intermittent errors
  • Tests for the changes have been added/updated (for bug fixes/features)
  • Docs have been added/updated (for bug fixes/features)
  • Best efforts were made to ensure docs/code are concise and coherent (checked for spelling/grammatical errors, commented out code, debug logs etc.)
  • Self-reviewed and ran all changes on a local instance (for bug fixes/features)

Change has impacts in these area(s)

(check all that apply)

  • Product design
  • Frontend

Describe the reason for change

Rect3Point drawing tool does not work in relative coordinates mode.
ff_front_dev_2132_rotating_bounding_box - to enable Rect3Point tool for <Rectangle/> control tag
fflag_fix_front_dev_3793_relative_coords_short - to enable relative coordinates mode for <ImageView/> object tag.

There are also some problems with Rect3Point itself. Some edge cases can provide unexpected NaN values.

What does this fix?

This PR allows Rect3Point to work in relative coordinates and fix the mathematics of drawing.

What libraries were added/updated?

@heartexlabs/ls-test

Does this change affect performance?

Nope

Does this change affect security?

Nope

What alternative approaches were there?

N/A

What feature flags were used to cover this change?

fflag_fix_front_lsdv_4673_rect3point_relative_310523_short
and related:

  • ff_front_dev_2132_rotating_bounding_box
  • fflag_fix_front_dev_3793_relative_coords_short

Does this PR introduce a breaking change?

(check only one)

  • Yes, and covered entirely by feature flag(s)
  • Yes, and covered partially by feature flag(s)
  • No
  • Not sure (briefly explain the situation below)

What level of testing was included in the change?

(check all that apply)

  • e2e
  • integration
  • unit

Which logical domain(s) does this change affect?

Rect3Point

@codecov-commenter
Copy link

codecov-commenter commented Jun 7, 2023

Codecov Report

Patch coverage: 66.11% and project coverage change: +0.30 🎉

Comparison is base (2d2d044) 65.79% compared to head (cf36d67) 66.10%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1425      +/-   ##
==========================================
+ Coverage   65.79%   66.10%   +0.30%     
==========================================
  Files         436      436              
  Lines       27316    27360      +44     
  Branches     7253     7265      +12     
==========================================
+ Hits        17973    18086     +113     
+ Misses       8027     7970      -57     
+ Partials     1316     1304      -12     
Impacted Files Coverage Δ
src/tags/object/Image/ImageSelection.js 68.11% <0.00%> (ø)
src/mixins/DrawingTool.js 81.62% <28.57%> (+15.83%) ⬆️
src/regions/RectRegion.js 76.52% <46.96%> (+6.41%) ⬆️
src/components/ImageView/Image.js 24.24% <100.00%> (+4.88%) ⬆️
src/mixins/Regions.js 74.59% <100.00%> (ø)
src/regions/EllipseRegion.js 84.61% <100.00%> (ø)
src/regions/KeyPointRegion.js 83.33% <100.00%> (ø)
src/regions/PolygonPoint.js 58.38% <100.00%> (ø)
src/regions/PolygonRegion.js 70.04% <100.00%> (ø)
src/tags/control/Rectangle.js 100.00% <100.00%> (ø)
... and 3 more

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Comment on lines 502 to 503
const maxStageWidth = isFF(FF_DEV_3793) ? 100 : self.obj.stageWidth;
const maxStageHeight = isFF(FF_DEV_3793) ? 100 : self.obj.stageHeight;
Copy link
Contributor

Choose a reason for hiding this comment

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

can you put the 100 in a variable and a comment to help explain why 100

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that 100 is used in src/regions/RectRegion.js aswell - would it make sense to define it as a constant value so we can reuse it where needed - would help the code be more readable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yyassi-heartex That's a great Idea. I hope I placed this constants in the right place.

@Gondragos Gondragos merged commit 6a8b379 into master Jun 13, 2023
@Gondragos Gondragos deleted the fb-lsdv-4673/rect3point-relative branch June 13, 2023 15:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants