Skip to content

[graf] Additional options for TPad::PlaceBox() method #12066

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.

Sign up for GitHub

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

Jump to bottom
Merged
guitargeek merged 1 commit into from
Jan 25, 2023
Merged

[graf] Additional options for TPad::PlaceBox() method #12066

guitargeek merged 1 commit into from
Jan 25, 2023

Conversation

@will-cern
Copy link
Contributor

@will-cern will-cern commented Jan 19, 2023 •
edited
Loading

This is needed so that the new RooBrowser doesn't need to access the private collision grid of the TPad. These additional options allow the placement of the box with choice of priority over directions as well as option to place within the margins of the pad.

Wanted for 6.28 release too!

@phsft-bot
Copy link

phsft-bot commented Jan 19, 2023

Can one of the admins verify this patch?

@jalopezg-git jalopezg-git added this to the 6.28/00 milestone Jan 20, 2023
jalopezg-git
jalopezg-git previously requested changes Jan 20, 2023
View reviewed changes
@will-cern will-cern mentioned this pull request Jan 23, 2023
Merged
@jalopezg-git jalopezg-git changed the title Additional options for PlaceBox method [graf] Additional options for TPad::PlaceBox() method Jan 24, 2023
@guitargeek guitargeek force-pushed the placebox_withinMargins branch from 10be79c to fe0e91b Compare January 24, 2023 21:14
@guitargeek
Copy link
Contributor

guitargeek commented Jan 24, 2023

Hi! We really need to speed things up to converge to the release, so I took the liberty of updating this PR with a revised version of the new TPad::PlaceBox() method, as requested by @jalopezg-git.

If the CI passes, please also have a look @couet to give also your approval as the code owner of this area.

@guitargeek
Copy link
Contributor

guitargeek commented Jan 24, 2023

@phsft-bot build

@phsft-bot
Copy link

phsft-bot commented Jan 24, 2023

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac12/noimt, mac11/cxx14, windows10/cxx14
How to customize builds

@phsft-bot
Copy link

phsft-bot commented Jan 24, 2023

Build failed on ROOT-ubuntu18.04/nortcxxmod.
Running on sft-ubuntu-1804-2.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Errors:

  • [2023-01-24T21:25:14.217Z] FAILED: tmva/sofie/test/CMakeFiles/SofieCompileModels_ONNX.util

@phsft-bot
Copy link

phsft-bot commented Jan 25, 2023

Build failed on windows10/cxx14.
Running on null:C:\build\workspace\root-pullrequests-build
See console output.

Failing tests:

@guitargeek guitargeek force-pushed the placebox_withinMargins branch from fe0e91b to 3581038 Compare January 25, 2023 11:50
@guitargeek
Copy link
Contributor

guitargeek commented Jan 25, 2023

@phsft-bot build

@phsft-bot
Copy link

phsft-bot commented Jan 25, 2023

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac12/noimt, mac11/cxx14, windows10/cxx14
How to customize builds

@phsft-bot
Copy link

phsft-bot commented Jan 25, 2023

Build failed on ROOT-ubuntu18.04/nortcxxmod.
Running on sft-ubuntu-1804-3.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Errors:

  • [2023-01-25T12:01:17.964Z] FAILED: tmva/sofie/test/CMakeFiles/SofieCompileModels_ONNX.util

@couet
Copy link
Member

couet commented Jan 25, 2023

Looks good to me. Does the legendautoplaced example on this page still works the same way after these changes?

@guitargeek
Copy link
Contributor

guitargeek commented Jan 25, 2023

Thanks Olivier! Running the example code with the PR branch I get this plot:
plot
It looks the same as in ROOT master.

2 similar comments
@guitargeek
Copy link
Contributor

guitargeek commented Jan 25, 2023

Thanks Olivier! Running the example code with the PR branch I get this plot:
plot
It looks the same as in ROOT master.

@guitargeek
Copy link
Contributor

guitargeek commented Jan 25, 2023

Thanks Olivier! Running the example code with the PR branch I get this plot:
plot
It looks the same as in ROOT master.

@guitargeek guitargeek dismissed jalopezg-git’s stale review January 25, 2023 12:28

Requested changes implemented

@guitargeek guitargeek mentioned this pull request Jan 25, 2023
Closed
52 tasks
@guitargeek guitargeek force-pushed the placebox_withinMargins branch from 3581038 to a632887 Compare January 25, 2023 12:43
jalopezg-git
jalopezg-git approved these changes Jan 25, 2023
View reviewed changes
Copy link
Contributor

@jalopezg-git jalopezg-git left a comment

Choose a reason for hiding this comment

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

See comments, otherwise LGTM!

@guitargeek guitargeek force-pushed the placebox_withinMargins branch 3 times, most recently from e286b75 to c14b1bc Compare January 25, 2023 14:09
@guitargeek
Copy link
Contributor

guitargeek commented Jan 25, 2023

@phsft-bot build

@phsft-bot
Copy link

phsft-bot commented Jan 25, 2023

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac12/noimt, mac11/cxx14, windows10/cxx14
How to customize builds

jalopezg-git
jalopezg-git reviewed Jan 25, 2023
View reviewed changes
@phsft-bot
Copy link

phsft-bot commented Jan 25, 2023

Build failed on ROOT-ubuntu18.04/nortcxxmod.
Running on sft-ubuntu-1804-2.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Errors:

  • [2023-01-25T15:09:33.218Z] FAILED: tmva/sofie/test/CMakeFiles/SofieCompileModels_ONNX.util

@will-cern @guitargeek
[graf] Additional options for TPad::PlaceBox() method
bf4a2ca 
This is needed so that the new RooBrowser doesn't need to access the
private collision grid of the TPad. These additional options allow the
placement of the box with choice of priority over directions as well as
option to place within the margins of the pad.

Wanted for 6.28 release too!
@guitargeek guitargeek force-pushed the placebox_withinMargins branch from c14b1bc to bf4a2ca Compare January 25, 2023 15:43
@phsft-bot
Copy link

phsft-bot commented Jan 25, 2023

Build failed on windows10/cxx14.
Running on null:C:\build\workspace\root-pullrequests-build
See console output.

Failing tests:

@phsft-bot
Copy link

phsft-bot commented Jan 25, 2023

Build failed on mac11/cxx14.
Running on macphsft20.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

@guitargeek guitargeek merged commit 77d8ab2 into root-project:master Jan 25, 2023
@guitargeek guitargeek mentioned this pull request Jan 25, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@guitargeek guitargeek guitargeek left review comments

@couet couet couet approved these changes

@Axel-Naumann Axel-Naumann Awaiting requested review from Axel-Naumann

@pcanal pcanal Awaiting requested review from pcanal pcanal is a code owner

+1 more reviewer

@jalopezg-git jalopezg-git jalopezg-git approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@couet couet

Labels

improvement in:Graphics

Projects

None yet

Milestone

6.28/00

Development

Successfully merging this pull request may close these issues.

5 participants

@will-cern @phsft-bot @guitargeek @couet @jalopezg-git