Closed Bug 895369 Opened 7 years ago Closed 2 years ago
Add asserts to Thebes
ThebesLayerBuffer is an absolute mess. It handles buffer rotation and non-rotated buffers, Azure or Thebes drawing, component alpha and regular buffers; it interacts with single and double buffered content clients in odd ways; optionally works with texture clients (I guess that is only for OMTC, but seems to be not always). Many of these paths do not work with each and there is little to ensure internal consistency. For example, using Azure with a single buffered content client crashes. Azure and component alpha don't work together. There are overloaded methods and weird implicit dependencies. BeginPaint is one of the hairiest methods I've ever seen, and there is some pretty stiff competition around. I believe buffer rotation might be going away at some point? Presumably once we have tiled thebes layers everywhere. That might take a while. It is probably worth deciding whether we should refactor ThebeLayerBuffer before or after that. But we definitely should improve it. In the mean time I'm going to insert as many assertions as I can.
I'm not going to start refactoring, but here are some asserts to make it a little tiny bit more obvious when we go wrong sometimes.
Attachment #778127 - Flags: review?(matt.woodrow)
Attachment #778127 - Flags: review?(matt.woodrow) → review+
Thanks for the asserts, that will help. Long term, we're getting rid of Thebes, would this refactoring be a first step to make that easier, or can we skip this bug if Thebes disappears?
I think we'll still need this class, or at least something that provides the equivalent functionality. A new name at least would be good. We currently have lots of branches for azure vs thebes. All of those can go away once we have azure everywhere.
(In reply to Milan Sreckovic [:milan] from comment #2) > Thanks for the asserts, that will help. Long term, we're getting rid of > Thebes, would this refactoring be a first step to make that easier, or can > we skip this bug if Thebes disappears? Yeah, what Matt said. This is another class called ThebesSomething even though it is not part of Thebes and doesn't even rely on Thebes specifically.
Nick, does it make sense to keep this bug still open?
I have no idea sorry. I haven't worked in the area for more than five years. Perhaps Matt Woodrow or Nical know?
Thebes/Content layers is not the nicest area of the code but Ryan improved it quite a bit.
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Summary: Refactor ThebesLayerBuffer → Add asserts to ThebesLayerBuffer
You need to log in before you can comment on or make changes to this bug.