Kiena's picture

NativeWindow

Project:The Open Toolkit library
Version:0.9.x-dev
Component:Code
Category:feature request
Priority:normal
Assigned:Kiena
Status:closed
Description

The main topic of this request for change/feature is the new OpenTK.NativeWindow class which will expose the internal INativeWindow implementations at a lower level than GameWindow, and will actually become latter's base class.

The extraction of INativeWindow specific details of GameWindow at the same time provided an opportunity for cleanup and made unavoidable the revision of several implementation and even design details.

The main purpose of this request is to let everyone interested and/or affected by these changes to provide feedback and participate in the decision making.

Details in the following posts.


Comments

Comment viewing options

Select your preferred way to display the comments and click "Save settings" to activate your changes.
Kiena's picture

#1

The included first draft is compilable, but is quite far from the intended final version.

It's hopefully sufficient for providing a correct overview of what these classes are to become like.

A few TODOs in the GameWindow class are from the version in the trunk, but the numerous new ones in both classes are notifications for unresolved details, and all should be resolved in a way that is acceptable for as many users of the library as possible, before inclusion in the official version. The possible changes not marked in the source will be included in following posts.

Notes for this version:

  • The reason for the current name of NativeWindow (NativeWindow0) is to ease integration of the class into the sources gotten from CVS. Platform specific code refers to Forms.NativeWindow and this intermediate workaround spares everyone from patching those classes too.
  • The first short test already revealed the appearance of an exception when the GameWindow is closed using the native close button (in short, X), but the main priority for now was to present the code for review, that issue should be fixed soon after some control flow examination.
  • (Sources removed due to the addition of newer versions.)

the Fiddler's picture

#2

Thank you, I'm studying the code as we speak.

The shutdown issue seems to be caused by a loop in the shutdown logic: the 'X' button is pressed, the Closing is fired, the window closes and the Closing event is fired again. This is probably caused by the Windows INativeWindow implementation - investigating.

Kiena's picture

#3

The two most important pending details not immediately visible in the sources (unlike typos right after posting):

  1. The purpose and usage of protected virtual On* methods:
    For historical reasons (because samles and documentation say so) the On* methods are invoked along with rising the corresponding events instead of the events being raised by them. Problems with this approach:
    • The library is intended to follow the .NET design guidelines, and they declare the purpose of On* methods as being the place the corresponding events are raised (allowing filtering or argument manipulation, for example).
    • The order in which a given event is raised and its matching On* method is invoked isn't consistent in the current implementation of GameWindow. This would obviously be resolved by the events being raised inside the On* methods.
    • The current implementation of GameWindow relies on some calls to On*Internal methods (which serve as invokers of both events and On* methods), which may not be worked around without adding some NativeWindow internals that are for GameWindow only. Simply calling the non-internal On* method wouldn't rise the corresponding event.

    What the foreseeable effects of the change would be, depending on their current usage:

    • For those who rely only on the On* methods, by overriding them:
      No noticeable change in behavior is expected, but the actual difference would be that the corresponding event doesn't get raised. (Absolutely minimal difference in performance difference, because either no handler, or an empty nameless method is attached to them.)

    • For those relying only on the raised event by adding handlers:
      No noticeable change in behavior is expected, because if not overridden, the On* the on method raises the event.
    • For those relying on both features:
      It's expected the events won't be raised due to their invocation being overridden with application logic. The solution would be to either call the base method as the first or last action in the overriding method or to consistently rely on only one of these features.
  2. Untimely raise of events ([Enable|Disable]Events):
    GameWindow attached its internal event handlers (to propagate them to the above mentioned On* methods and events visible on GameWindow) to its INativeWindow at the end of its construction, but because of the new model involving inheritance, the events exposed by GameWindow actually belong to the base NativeWindow class, and normally would get connected to their handlers before the derived constructor even starts.

    This could easily cause crashed if the operating system sends a window event and for example logic involving OpenGL operations has been placed into the On* method the event is propagated to.

    My solution proposal is the addition of the protected EnableEvents and DisableEvents to NativeWindow which would allow both the delaying, both the stopping of propagation of events arriving from the underlying system. (If the discussed change in the previous point was accepted, it would allow manual event raising by calling On* methods under all circumstances (unless specifically overridden not to do so).)

    In case early event calls would cause no problems for an application it would be kind of uncomfortable and easy to forget to actually call EnableEvents, for this reason my proposal is the addition of a boolean parameter to NativeWindow constructors that allows events being enabled without any further actions being required.

A very small update of the 1st draft is included because the NativeWindow constructor didn't include the consideration of the mentioned parameter.

(Sources removed due to their addition to the repository.)

the Fiddler's picture

#4

Status:open» in progress

I have created a new branch for the NativeWindow updates. Before we continue, please check out from svn://opentk.svn.sourceforge.net/svnroot/opentk/branches/nativewindow. Future changes should go to that branch - this will make our lives much easier in the long run.

I have committed your latest version to the new branch and fixed the shutdown issues. Changes:

  • Made GameWindow.Exit() an alias of GameWindow.Close().
  • Modified NativeWindow.Close() to call implementation.Close() instead of Dispose(). This provides that the same shutdown logic in all code paths (i.e. the Closing is always called, etc).
  • Completely removed the ugly GameWindowExitException hack. The GameWindow loop will now exit as soon as the NativeWindow is closed - this is a breaking change for applications that expect GameWindow.Exit() to terminate immediately. These applications should throw an exception to ensure correct termination.
  1. The purpose and usage of protected virtual On* methods:
    Considering our options, it seems best to follow the design guidelines and raise events from the On* methods. This is a breaking change, but you are correct that it's impact to existing is minimal.
  2. Untimely raise of events ([Enable|Disable]Events):
    This is a more hairy issue. I do not really like having separate methods to enable/disable events explicitly - this is neither intuitive nor discoverable for the user.

    One potential solution would be to ensure that events are *only* raised once NativeWindow.ProcessEvents() is called at least once. This can be done easily by having NativeWindow.ProcessEvents() use EnableEvents() on first call (almost the same as your proposal, with the difference that EnableEvents() is called implicitly.)

Kiena's picture

#5

NativeWindow0->NativeWindow renaming patch.
(Has been applied to the branch.)

Kiena's picture

#6

[Enable|Disable]Events:
I suggest the following changes:

  • protected bool OSEvents { get; set; } instead of [Enable|Disable]Events methods. I wouldn't make the property internal because disabling event propagation should be needed during derived class (e.g. GameWindow) disposal if delaying of events was desired during construction. Maybe events could be automatically disabled at a reasonable point as well (Close?), but there may be cases e.g. a context change for some reason, when this level of control could be useful.
  • public void ProcessEvents() enables event propagation if it currently is not, whenever called.
  • protected void ProcessEvents(bool retainOSEvents) method which is optionally usable to consume OS events, yet preserve propagation state. It is called with the default false argument by its public counterpart. If the argument is true then propagation won't be disabled, instead it will remain whatever it was like before the call.

EDIT:

  • Constructors with the extra bool argumenst will be gone, of course.

Like this users of the library should not be aware of these features unless they actually need them, I believe.

Note: Suggestions of better names for the OSEvents property and retainOSEvents variable are welcome.

Disabling events automatically in the Close method or at another point is a new idea, and should be decided after the proper place for it in GameWindow has been found.

the Fiddler's picture

#7

Committed NativeWindow0 -> NativeWindow patch.

I am running some tests to determine whether events are likely to cause issues for inheriting classes.

Edit: I haven't encountered any adverse side-effects by enabling events inside the NativeWindow constructor. As far as I can tell, events are only generated when INativeWindow.ProcessEvents is called. This method is not called by a the NativeWindow constructor, so no events will be generated unexpectedly.

Will have to verify this holds for all platforms, however.

Kiena's picture

#8

This patch needs review and some debugging before it could be committed.

The issue appears at the same spot as before (usage of native close button), but is a different kind.

Included:

  • [Enable|Disable]Events -> Events property (instead of OSEvents, as derived classes may listen to it as well)
  • Added restoration of screen resolution to NativeWindow's Dispose, if it was created in fullscreen mode. It should be tested.
  • *Cleaned up event propagation in both classes. There shouldn't be breaks caused beyond those discussed previously by the NativeWindow parts, but the GameWindow changes include some paradigm changes (updated the methods' documentation to reflect them). (The visibility of two On* methods in GameWindow remained public for compatibility reasons.) In worst case the semantics of On*[Internal] methods in GameWindow could be restored to trunk's.
  • *Added implicit event control to ProcessEvents and Close, and removed the constructors including the bool parameter.
  • *Setting of isExiting was added to Exit.
  • *Cleaned up Run somewhat and removed some unused internals. Revision is necessary.

I couldn't determine yet which of these changes resulted in the exception thrown during buffer swapping. (Though the same rendering function seems to pass some GL operations without problems (swallowed exceptions?), otherwise it should be the case that rendering is still invoked after the context/native window becomes invalid, when the window is closed this way.) I marked the problem source candidates with *, regrettably couldn't exclude the majority of changes...

(Updated patch in a following post.)

the Fiddler's picture

#9

Comments:

  • If we can guarantee that events are only raised inside INativeWindow.ProcessEvents, there will not be a need to add the Events property (needs review). I agree we should make a strong guarantee for event generation (e.g. no events in constructor), but I think the global event toggle should be avoided if possible.
  • INativeWindow.Dispose() may be called *after* the window has been closed. Calling WindowState = WindowState.Normal will cause problems in that case. A call to RestoreResolution() should suffice.
  • OnLoad/OnUnload should become protected, even if this breaks compatibility. Better leave that to a separate patch, however, as it requires wide changes to Examples project.
  • Good catch on isExiting = true.
  • Rendering after context becomes invalid: GameWindow used to throw a GameWindowExitException to ensure this never happened. This was bad design for many reasons.

    One potential solution is to make GameWindow.Exit()/Close() work asynchronously. Both could simply set isExiting to true and the actual base.Close() could be moved right after the main loop (inside the run method).

    This still leaves a potential issue of calling Dispose() while the main loop is running, however. Unless we can find a reliable solution for shutting down the main loop, maybe we'll have to reintroduce the internal GameWindowExitException.

  • The run method needs a general cleanup, but it's best to leave it mostly unchanged for the purposes of this issue (i.e. no changes to actual logic, if possible).

I'll be without internet access until Monday, so I won't be able to review/commit patches for a few days.

Kiena's picture

#10

There definitely is a solution without using the exception for flow control.

Something potentially related unresolved detail not mentioned so far:

Should NativeWindow's Dispose be virtual, and should GameWindow override it? The addition of IDisposable to GameWindow's signature guarantees its Dispose method being called at the end of using blocks, but depending on the variable type it's possible only NativeWindow's is run in a try-finally block.