Skip to content
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.

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

Splitting Platform/Windows/API.cs into OpenTK.NT #765

Merged
merged 90 commits into from
Jul 21, 2018

Conversation

FreezyLemon
Copy link
Contributor

@FreezyLemon FreezyLemon commented Jun 5, 2018

THIS IS HEAVILY WIP!

Todo

  • General cleanup pass
  • Check for namespace errors (all should be OpenTK.NT.Native, and not e.g. OpenTK.NT.Native.User32.Structs)
  • Standardize type names with Win API (-> Structs)
  • Use windows typedef names (e.g. HWND instead of IntPtr)
  • check naming (especially PascalCase, unless we want to stick to the Win API with those too)
  • make (almost) everything public
  • [/] create more wrappers for function calls that could use them (postponed)

@VPeruS
Copy link
Contributor

VPeruS commented Jun 8, 2018

What is end purpose of this?

@FreezyLemon
Copy link
Contributor Author

Moving all the P/Invoke code into its own library..
Check this blog post as a reference.

Remove unused leftover functions
- Renamed structs to be inline with WinApi docs
- Used WinApi typedefs whereever(?) possible (might have missed some)
- renamed Enums to be more consistent (uppercase if part of a struct, if only used with a function, '<FunctionName><Usage>' in PascalCase instead)
- added docs from WinApi docs for a lot of enums
@FreezyLemon
Copy link
Contributor Author

The files directly in the User32 folder (Cursor, Device, etc...) still need some refactoring regarding windows typedefs (when I did those, I did not replace every type with the windows type def).
Except for User32 and the two unchecked points above (wrapper are low priority), I'm pretty much done.

EDIT: I still need to clean up the remaining code in API.cs, I'd love to get rid of it completely. Everything in the class "Constants" should be moved to enums or flags. The non-WinApi structs should be moved to OpenTK.Core->Platform->Windows.
Not sure what to do about the #if ANDROID || IPHONE || MINIMAL segment though.

@FreezyLemon
Copy link
Contributor Author

FreezyLemon commented Jul 11, 2018

I went with the following:

  • Leave undocumented members in, but mark them as such in the XML documentation ("This member is not officially documented. Use with care.")
  • Delete members that are no longer present in the header files (we can assume that those are not supported anymore)

There's still the things from my last "todo" post (testing some structs) plus a very small amount of missing XML doc (<25 stylecop errors), but this is mostly done now.
edit: I'll also do a pass over the whole thing once to check if I missed some old XML documentation.

@Nihlus
Copy link
Contributor

Nihlus commented Jul 11, 2018

Absolutely fantastic. Looking forward to merging this.

@@ -51,53 +49,8 @@ public IGraphicsContext CreateContext(int major, int minor, GraphicsContextFlags
return new GraphicsContext(mode, WindowInfo, major, minor, flags);
}

public bool IsIdle => !PeekMessage(ref msg, IntPtr.Zero, 0, 0, 0);
public bool IsIdle => !User32.Message.PeekMessage(out message, IntPtr.Zero, 0, 0, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure that message are allocated in managed code before call? Last time got issue by passing huge struct with out mod(it was nested and with arrays) and it genereated runtime exceptions, always with random time after call while trying to Marshal struct, changes to ref with preallocation before call fixed issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since message is a field, it will always be initialized before we access it.
This means that
private Msg message; is (at least for normal usage) equivalent to
private Msg message = default(Msg);

So yes, I'm pretty sure that the message is allocated before the unmanaged function call.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok : )

Copy link
Contributor

Choose a reason for hiding this comment

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

What if someone make call:
Msg msg;
PeekMessage(out msg);

Copy link
Contributor

@Nihlus Nihlus Jul 15, 2018

Choose a reason for hiding this comment

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

msg will be initialized to its default value. Even then, with an out, you don't strictly need to initialize anything since out guarantees initialization. Might want to brush up on that.

@Nihlus
Copy link
Contributor

Nihlus commented Jul 21, 2018

Looks like it's just about time to merge this. All set, @FreezyLemon?

@Nihlus Nihlus merged commit 49b4125 into opentk:4.0 Jul 21, 2018
@FreezyLemon FreezyLemon deleted the 4.0-split-nt branch July 21, 2018 21:47
@FreezyLemon FreezyLemon changed the title [WIP] Splitting Platform/Windows/API.cs into OpenTK.NT Splitting Platform/Windows/API.cs into OpenTK.NT Jul 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants