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

glfw: Window hints rework #71

Merged
merged 21 commits into from Nov 16, 2021
Merged

glfw: Window hints rework #71

merged 21 commits into from Nov 16, 2021

Conversation

InKryption
Copy link
Contributor

@InKryption InKryption commented Nov 11, 2021

I took a different approach to the one used for initialization hints, because I realized that it would probably a lot of overhead to call Window.hint for every possible hint, compared to how status quo allows one to avoid that overhead; as well, it would probably be a lot harder to maintain, given that, unlike InitHint, window initialization hints are of more types than just booleans.
This approach is (quite a bit) more verbose, but avoids the overhead, and probably makes it easier to maintain.

Also, I found that context_revision and context_no_error are listed in Window.Hint, but are not listed as hints in the docs, only as attributes; I commented them out to make what I have pass tests - is this intentional and I'm missing something, or is this the result of some mishap?

Edit: it should also be noted that unlike the change made to initialization hints, I didn't completely re-use the in-place API, because I ran into some issues regarding the type-inference in the Window.hint function, so you'll see I ended up re-implementing the code for Window.hint in HintValue.set (where HintValue is a tagged union, with a tag type of Hint).

Edit: as pointed out by @silversquirl, there isn't actually a lot of overhead involved in the struct approach, so now the union slice is no more. Now just to address the other components of this problem, such as Window.hint, which seems to have some some type inference issues which caused compilation to fail under this new model. Should we try and rework it, or discard it in favor of Hints.set (private function)?

Edit: Closes #65

  • By selecting this checkbox, I agree to license my contributions to this project under the license(s) described in the LICENSE file, and I have the right to do so or have received permission to do so by an employer or client I am producing work for whom has this right.

@InKryption InKryption changed the title Window hints rework glfw: Window hints rework Nov 11, 2021
@silversquirl
Copy link
Contributor

it would probably a lot of overhead to call Window.hint for every possible hint

I had this thought initially too, but if you look at the GLFW source, it's probably about the same as calling glfwDefaultWindowHints. I'd say have a hints struct with default values, and just set all of them unconditionally.

If you do want to avoid setting hints that don't need changed though, I'd still recommend going the struct route, and just check if the value is different than the default. This is easy to do by just looping over std.meta.fields, since the typeinfo gives you default values.

A struct results in a nicer API than an array of hints - the latter still feels very much like a C API, not a Zig one.

@InKryption
Copy link
Contributor Author

@silversquirl Good point. I don't know why I didn't actually check the source; guess that's what I get for starting this on my laptop at a café. And I guess that's also why I made this a draft.

@InKryption
Copy link
Contributor Author

InKryption commented Nov 12, 2021

@silversquirl Do you have any thoughts on what to do with the currently unused Window.hint function?

@silversquirl
Copy link
Contributor

Either move the type conversion logic in set into it, or delete it. Not much point in keeping it if it's not used

glfw/src/Window.zig Outdated Show resolved Hide resolved
@InKryption
Copy link
Contributor Author

Excuse my fumbling with git, trying to make the history not look messy.

glfw/src/Window.zig Outdated Show resolved Hide resolved
glfw/src/Window.zig Outdated Show resolved Hide resolved
glfw/src/Window.zig Outdated Show resolved Hide resolved
…ecified to be a Window creation hint by the docs, and affirm removal of `Hint.context_revision`, which isn't.

The docs don't seem to specify a default value for `Hints.context_no_error` to take on, so we could set it based on `std.debug.runtime_safety` like this.
…xtRobustness`, `ContextReleaseBehavior`, and `OpenGlProfile` from consts.zig, and remove the now unused constants (replaced by aformentioned enum values).
@slimsag
Copy link
Member

slimsag commented Nov 12, 2021

Thanks for all your work on this! It'll probably be a few days before I have a chance to review (at handmade Seattle and then to a wedding) so apologies for the delayed review!

@InKryption
Copy link
Contributor Author

I had an idea to maybe add a test that compares the attribute values of a window created with Window.from(c.glfwCreateWindow(...)), to the attribute values of a window created with Window.create(...) (in that order), to check that, for any attribute whose value is initialized via Window.Hints.set, its value is the same as what would be set by GLFW itself (maybe excluding context_no_error). This wouldn't be full-coverage, but it would be something. Thoughts?

@slimsag
Copy link
Member

slimsag commented Nov 15, 2021

@InKryption regarding:

I had an idea to maybe add a test that compares the attribute values of a window

This sounds like an obvious win to me, so yes, please do! I won't block this PR on this, please send another PR for it if I merge before you get to it.

One thing to be mindful of: if we want the test to pass on any OS, be sure to choose hints that work on all OS/platforms.

@InKryption
Copy link
Contributor Author

InKryption commented Nov 15, 2021

@slimsag Roger roger, I'll set this as ready for merging then, in the absence of any notes you may have.

@InKryption InKryption marked this pull request as ready for review November 15, 2021 22:44
Co-authored-by: Stephen Gutekanst <stephen.gutekanst@gmail.com>
glfw/src/Window.zig Outdated Show resolved Hide resolved
glfw/src/Window.zig Outdated Show resolved Hide resolved
…naming convention

Co-authored-by: Stephen Gutekanst <stephen.gutekanst@gmail.com>
glfw/src/Window.zig Outdated Show resolved Hide resolved
Co-authored-by: Stephen Gutekanst <stephen.gutekanst@gmail.com>
glfw/src/consts.zig Outdated Show resolved Hide resolved
Copy link
Member

@slimsag slimsag left a comment

Choose a reason for hiding this comment

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

Looks really great, thanks for sending this. Once my comments are addressed and tests pass I'll merge. Feel free to send the test in this PR or a separate one, whichever you prefer.

glfw/src/Window.zig Outdated Show resolved Hide resolved
glfw/src/Window.zig Outdated Show resolved Hide resolved
@slimsag
Copy link
Member

slimsag commented Nov 16, 2021

Looks good to me, let me know when you're finished and I'll merge. I will need to squash merge due to non-linear history, unless you want to rebase everything (I'm happy either way)

Copy link
Member

@slimsag slimsag left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a ton again!

@slimsag slimsag merged commit 28a0aeb into hexops:main Nov 16, 2021
@InKryption InKryption deleted the window-hints-rework branch November 16, 2021 01:43
@InKryption
Copy link
Contributor Author

Closes #65

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

glfw: Friendlier window creation
3 participants