Opened 4 years ago

Closed 3 months ago

Last modified 3 months ago

#12904 closed defect (fixed)

wxGCDC bounding box not updated

Reported by: Spoek Owned by: VZ
Priority: normal Milestone: 3.0.2
Component: GUI-all Version: stable-latest
Keywords: wxDC, bounding box, MaxY Cc:
Blocked By: Blocking:
Patch: yes

Description

Source

analyze font
wxBitmap testbmp;
wxFont f = fonts->GetFont(i);
sets to a specific font
wxMemoryDC dc;
dc.SelectObject(testbmp);
dc.SetFont(f);
dc.SetBackground(* wxWHITE_BRUSH);
dc.SetTextBackground(* wxWHITE);
dc.SetTextForeground(* wxBLACK);
dc.Clear();
dc.DrawText("H", 0, 0);
wxCoord height = dc.MaxY();

.....

Returns 0 for wxDC::MaxY() on wxMac v2.9.1. Works on wxMSW.

Attachments (3)

dc-test.patch download (2.9 KB) - added by Spoek 4 years ago.
A patch to add new tests for testing wxDC.
dodrawtext_mac_bbox_update.patch download (700 bytes) - added by toni 3 months ago.
Naive fix for just making `DrawText˙ calls update the bounding box. See comment:6
gcdc_bbox_updates.patch download (17.3 KB) - added by toni 3 months ago.
Adds missing bounding box updates to wxGCDC methods. Includes new unit tests. See comment:11

Download all attachments as: .zip

Change History (23)

comment:1 Changed 4 years ago by Spoek

error in source:

  • wxBitmap testbmp;

+ wxBitmap testbmp(100,100);

comment:2 Changed 4 years ago by vadz

Would be nice to add some tests/graphics/dc.cpp and add this test to it, any patches doing this would be welcome. I suppose that the colour setting is immaterial here, only setting the font and calling DrawText() should already update the bounding box.

Changed 4 years ago by Spoek

A patch to add new tests for testing wxDC.

comment:3 Changed 4 years ago by Spoek

  • Keywords wxDC bounding box MaxY added

Supplied a patch file for testing 2 things.
1) the bounding box after a DrawText().
2) GetPixel() which also doesn't seem to work on wxMac. Maybe I should make another ticket for this.

comment:4 Changed 4 years ago by vadz

  • Status changed from new to confirmed

I think GetPixel() can't be made to work at all so it's a "known limitation" rather than a bug (if I'm right about this it should definitely be documented though).

In any case thanks a lot for the test, I won't apply it now to avoid breaking the test suite before the bug is fixed but we'll definitely commit it when the bounding box computation is updated, this is exactly what was needed and will be very useful.

comment:5 follow-up: Changed 2 years ago by vadz

  • Component changed from wxOSX-Carbon to GUI-all
  • Summary changed from wxDC BoundingBox not updated after DrawText() on wxMac v2.9.1 to wxGCDC bounding box not updated
  • Version changed from 2.9.1 to 2.9-svn

The problem is actually that the bounding box is maintained at wxGraphicsContext level but wxGCDCImpl doesn't know anything about it. We could update the DC bounding box after each call but this would be both inefficient and require a lot of dull work as all methods would need to be modified.

It would be probably better to make MinX(), MaxX(), MinY() and MaxY() virtual or perhaps call some virtual UpdateBoundingBox() from all of them and override it in wxGCDCImpl to get the bounding box from the graphics object.

comment:6 in reply to: ↑ 5 Changed 3 months ago by toni

Replying to vadz:

The problem is actually that the bounding box is maintained at wxGraphicsContext level but wxGCDCImpl doesn't know anything about it.

As I needed the bounding box after DrawText calls on a Mac, I've made a small change to make it work for me. Patch attached (against trunk). The change is in common/dcgraph.cpp because of the separation you describe.

The above is actually my first delving into the wx code, and I'd like to try making a more general fix if you wouldn't mind mentoring me a bit. Do you have any new ideas on how this issue would be best addressed?

Changed 3 months ago by toni

Naive fix for just making `DrawText˙ calls update the bounding box. See comment:6

comment:7 Changed 3 months ago by vadz

The patch would work (and probably doesn't even need __WXMAC__ check around it, it should be the same thing under all platforms unless I'm missing something), but, again, this is not ideal.

I don't have any new ideas but I still think that the one from the comment:5 is the right thing to do, i.e. that we must make {Min,Max}{X,Y}() virtual in wxDCImpl and override them in wxGCDCImpl to call m_graphicsContext->GetBox() to really get the values. This looks simple enough to me and should fix the bug, what do you think?

comment:8 Changed 3 months ago by toni

The __WXMAC__ check is there because I got an updated bounding box on Windows as expected. However I now realize that I did so because wxGCDC was not used on Windows, but on Mac I got one from wxPaintDC implicitly.

What I still fail to understand is how can we calculate the bounding box of a drawing since the last call to wxDC::ResetBoundingBox if we don't update the bounding box after every drawing operation. Or did I miss wxGraphicsContext doing this already somehow?

comment:9 Changed 3 months ago by vadz

Yes, it does it, at least under Mac, see wxMacCoreGraphicsPathData::GetBox() in source:wxWidgets/trunk/src/osx/carbon/graphics.cpp#L1302.

comment:10 Changed 3 months ago by toni

That GetBox would only work where paths are involved, not for things like text and bitmaps for example. Also, the paths are not kept after an operation ends, so we could not call GetBox on them at the point of bounding box query calls.

As far as I can tell from the rest of the code, updating the bounding box during a drawing operation is the convention; even some wxGCDC methods already do this, like DoDrawRotatedText, DoDrawLine, DoDrawCheckMark (as it delegates to wxDCImpl) and DoCrossHair for example. IMHO we should keep things simple and consistently do the same in other methods. It doesn't seem to me that the performance penalty would be significant, as most of the time the required values for updating the bounding box are easily available.

After going though the drawing operations in dcgraph.cpp, I found that these are the ones that do not update the bounding box:

  • Bitmap
  • Icon
  • Arc
  • EllipticArc
  • Lines
  • Spline
  • Polygon
  • PolyPolygon
  • Rectangle
  • RoundedRectangle
  • Ellipse
  • Blit
  • StrechBlit
  • Text
  • GradientFillLinear
  • GradientFillConcentric

There isn't that many, and some of them only delegate to other more general ones. Logic for updating the bounding box would have three variations: (1) when it's based on already known points; (2) when it's based on a wxGraphicsPath::GetBox call; and (3) when it's based on a GetTextExtent call. The last one is the only one that makes me think for a minute about the significance of it's cost. An optimizing measure for the extreme use cases could maybe be adding a preprocessor switch for turning off bounding box updates if you don't need it. However, I wouldn't be inclined to add this prematurely.

comment:11 follow-up: Changed 3 months ago by vadz

OK, I didn't realize that this wouldn't work for text. Let's add the code to update it manually then everywhere -- looking forward to your patch!

If you could please also update the patch from comment:3 to do some more precise checks on the bounding box, it would be great as well.

Finally, I agree that calling GetTextExtent() might be problematic from performance point of view, however I also believe that we shouldn't worry about this right now and just add some wxDC::DisableBoundingBoxUpdates() later if it does turn out to be a problem.

Thanks again!

Changed 3 months ago by toni

Adds missing bounding box updates to wxGCDC methods. Includes new unit tests. See comment:11

comment:12 in reply to: ↑ 11 Changed 3 months ago by toni

I don't know what's the policy on unicode characters in source code. If there's an issue with the "ž" in my last name, replace it with a "z".

Replying to vadz:

Thanks again!

Happy to contribute :)

comment:13 Changed 3 months ago by vadz

  • Milestone set to 3.0.2
  • Patch set

Thanks! Looks great to me, I'll test and apply it a.s.a.p.

comment:14 Changed 3 months ago by VZ

  • Owner set to VZ
  • Resolution set to fixed
  • Status changed from confirmed to closed

In 76953:

Implement bounding box computations for wxGDDC.

Update the bounding box in all the methods drawing something. This wasn't
done before in many of them, resulting in the bounding box remaining empty,
but it is updated now and a new test checking that it is was added.

Closes #12904.

comment:15 follow-up: Changed 3 months ago by vadz

Committed with minor changes (renamed the test file because I think we may want to have other bounding box tests there later, e.g. for wxGraphicsContext or even wxDC itself, too).

Thanks once again!

comment:16 Changed 3 months ago by VZ

In 76954:

Implement bounding box computations for wxGDDC.

Update the bounding box in all the methods drawing something. This wasn't
done before in many of them, resulting in the bounding box remaining empty,
but it is updated now and a new test checking that it is was added.

Closes #12904.

comment:17 in reply to: ↑ 15 ; follow-up: Changed 3 months ago by toni

Replying to vadz:

Committed with minor changes (renamed the test file because I think we may want to have other bounding box tests there later, e.g. for wxGraphicsContext or even wxDC itself, too).

OK, you just forgot to modify the info in the file header.

In regards to my reasoning for it's naming, the setup of that test class is highly specialized for wxGCDC bounding box testing, so I figured it would be better to make other files for other test cases -- but I can understand wanting to keep the number of files from exploding.

comment:18 in reply to: ↑ 17 Changed 3 months ago by toni

Replying to toni:

so I figured it would be better to make other files for other test cases

It kinda slipped my mind that you could put more classes in the same file... :/

comment:19 Changed 3 months ago by VZ

In 76955:

No changes, just update the file name in the header.

Update the name in the header to match the actual file name.

See #12904.

comment:20 Changed 3 months ago by VZ

In 76956:

No changes, just update the file name in the header.

Update the name in the header to match the actual file name.

See #12904.

Note: See TracTickets for help on using tickets.