Skip to content

Conversation

@ferdymercury
Copy link
Collaborator

@ferdymercury ferdymercury commented Oct 22, 2025

This Pull request:

Changes or fixes:

Fixes #20138

fyi @rlalik

a

Checklist:

  • tested changes locally
  • updated the docs (if necessary)

@ferdymercury ferdymercury requested a review from couet as a code owner October 22, 2025 11:30
it was forgotten in the pad constructor
@ferdymercury ferdymercury changed the title [graf2d] allow zero-margins between pads [graf2d] allow zero-margins between pads and respect color for negative margins Oct 22, 2025
@github-actions
Copy link

github-actions bot commented Oct 23, 2025

Test Results

    22 files      22 suites   3d 17h 7m 53s ⏱️
 3 696 tests  3 671 ✅ 0 💤  25 ❌
79 361 runs  79 228 ✅ 0 💤 133 ❌

For more details on these failures, see this check.

Results for commit b6cbdf3.

♻️ This comment has been updated with latest results.

@rlalik
Copy link
Contributor

rlalik commented Oct 23, 2025

I didn't test the PR code but this seems to be the solution. However, since this was broken and is being fixed, I would like to propose some change. Current behavior is that, when setting non-zero positive margin, the margin is applied to all pads, also on the outer edges, not only between pads. But sometimes one wants to increase the margin to separate the pads, for better visibility or so, but actually we don't want to increase the margin on the outer edges, where there is no other pad to keep distance from.

So my proposition would be, when the margin is positive, have all margins set - this will be the default behavior, compatible with 99% existing code. But when the margin value is negative, only the inner margin between pads are set, and the outer margin are kept 0. Setting margin to be 0 removes the margins between pads.

A such implementation would be flexible I think for almost all cases.

So with canvas->Divide(2, 2, -0.02, -0.02) I would get something like this:
can_no_outer_margins
and with canvas->Divide(2, 2, 0.02, -0.02) I would get something like this:
can_no_outer_margins
Could it be done?

@ferdymercury ferdymercury added this to the 6.38.00 milestone Oct 24, 2025
@ferdymercury
Copy link
Collaborator Author

@rlalik would you like to propose a code modification for the new feature you are mentioning? It should go just below the if statement I modified

@rlalik
Copy link
Contributor

rlalik commented Oct 24, 2025

@rlalik would you like to propose a code modification for the new feature you are mentioning? It should go just below the if statement I modified

So I propose that this gets merged and then I submit PR with my proposed change to add my requested feature.

@ferdymercury ferdymercury requested a review from linev October 24, 2025 07:50
@linev
Copy link
Member

linev commented Oct 24, 2025

I reopen PR to restart CI

@linev linev closed this Oct 24, 2025
@linev linev reopened this Oct 24, 2025
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.

Improper behavior of TCanvas::Divide

3 participants