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

Input and Windowing via GLFW #867

Merged
merged 225 commits into from
May 26, 2019
Merged

Input and Windowing via GLFW #867

merged 225 commits into from
May 26, 2019

Conversation

daerogami
Copy link
Contributor

@daerogami daerogami commented Nov 22, 2018

This PR at the time of creation represents the initial prototype of using GLFW inplace of the legacy Platform code.
This decision is not final and purely serves as a springboard from which to spur discussions around viable and pragmatic improvements.
Lets have all general discussion occur in #866 whereas specific discussion around the implementation presented in this PR happen here.

Closes #824

Decomposed IGameWindow into smaller parts for readability;
Copied all objects referenced from interfaces;
Copy link
Member

@varon varon left a comment

Choose a reason for hiding this comment

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

Nice work, @daerogami. Looks like it's coming along decently. There's a lot of code added here, so consider this a super early non-judgemental review.

  • I'd suggest doing a separate PR for OpenToolkit.Input first if you need it, things like Key, Mouse, etc should all live in that project. Handling them one at a time makes reviewing much easier.

We could also trim this down by swapping out the license headers to the new one:

//
// <filename>
//
// Copyright (C) <year of creation> OpenTK
//
// This software may be modified and distributed under the terms
// of the MIT license. See the LICENSE file for details.
//

via @Nihlus in #829

@varon varon changed the title [WIP] [4.0] Begin moving Windowing/Input/Platforms to OpenToolkit.Windowing [WIP] [4.0] Prototype Windowing via GLFW Nov 23, 2018
@varon varon added this to To Do in OpenTK 4.0 Nov 23, 2018
@varon varon moved this from To Do to In Progress in OpenTK 4.0 Nov 23, 2018
@varon
Copy link
Member

varon commented Nov 23, 2018

also, watch out for potential duplicates via #861!

@daerogami
Copy link
Contributor Author

also, watch out for potential duplicates via #861!

Will probably make sense for me to rebase off of that branch since maintaining the current GameWindow API heavily depends on existing Input. Not sure how this will tie into GLFW since it handles input as well, haven't researched that far ahead.

@Perksey Perksey added this to the 4.0.0 milestone Nov 25, 2018
@Perksey
Copy link
Contributor

Perksey commented Nov 27, 2018

Can't help but notice this doesn't implement the whole API. For it to be viable to replace the platforms of OpenTK, we'll need the whole API. See http://www.glfw.org/docs/latest/

@varon
Copy link
Member

varon commented Nov 27, 2018

@DylanFPS - I'm not sure sure we want to expose the API at all. The purpose of this prototype is to determine if we can replace the current GameWindow (which doesn't expose native functionality) functionality with GLFW.

@Perksey
Copy link
Contributor

Perksey commented Nov 27, 2018

Yeah but afaik the contents of this PR isn't enough to match the current GameWindow API (mainly the functions required for input)

@Perksey
Copy link
Contributor

Perksey commented Nov 27, 2018

Also @varon why wouldn't we want to expose the entire API? Overachievers/people who want to follow C tutorials exactly might want to veer away from our GameWindow API. While we can all call them nuts for doing so, I wouldn't want to block it completely. Moreover, by taking just what we need, we're restricting any posibility that there might be something that the user wants from GLFW or GameWindow that they can't have because we simply haven't implemented it. I think we should have this module complete to the best of our abilities.

@Nihlus
Copy link
Contributor

Nihlus commented Nov 27, 2018

The question we musk ask ourselves is the intent behind the action. Are we implementing bindings to GLFW for use by us, or are we providing them as a service to the end user?

I'm inclined to choose the former, at least for now, since it reduces the amount of work needed in the present and allows us to move faster with the new GameWindow implementation. Further, if we intend on providing them to the end user, the focus of the PR should be the bindings themselves, and the GameWindow's use of them should be at best a secondary objective.

Both views have merit, of course, but given the current situation I believe that our energy is best spent on a smaller, internal binding for our own use at this point. Once we know we can use it, and once we know what our infrastructure will look like, we can expand the system to a full-scale user-consumable one.

@Perksey
Copy link
Contributor

Perksey commented Nov 27, 2018

With the release workload in mind, I think we should only include the bindings required for GameWindow - our main objective at this point should be preserving the API. However, I strongly agree that we should revisit implementing the complete API for a later module release. (I think we should create an issue for this after merge)

@daerogami
Copy link
Contributor Author

The intent is to carry over INativeWIndow and IGameWindow and implement as much of it as possible using GLFW bindings. I think we are all on the same page on that part. We still need to figure out what this means for inputs, I don't know if we can ignore GLFW's input device features and roll with the legacy stuff because it pulls from platforms which this PR is trying to help us remove.

@Perksey
Copy link
Contributor

Perksey commented Nov 28, 2018

GLFW should be able to do most of the job, including input so we do need those APIs. Hopefully by the time GameWindow is fully rebuilt there’s no need for platforms.

Pulled in more related GLFW bindings;
@varon
Copy link
Member

varon commented May 3, 2019

How are we looking here? Ready for that review?

@devvoid
Copy link
Contributor

devvoid commented May 3, 2019

Yes, but I won't be able to act on that review for another week or two, it's almost finals week.

@devvoid
Copy link
Contributor

devvoid commented May 13, 2019

I'm considering whether or not we should just get rid of the windowing tests - so long as ADL functions and GLFW doesn't have any massive API-breaking updates, I don't see this as being something that would break in an OpenTK update. Plus, everything we've tried to get them to work on the CI just doesn't work.

@jvbsl
Copy link
Contributor

jvbsl commented May 13, 2019

As far as I can tell as long as my PR for ADL gets merged(still need to get the tests to work there as well :D Tests seem to be my enemies in OpenTK) the tests I have in a branch of mine should work on Linux and Windows as well. But I need to find enough time to do it.

@varon
Copy link
Member

varon commented May 14, 2019

Windowing is a nightmare to test, especially on CI. I strongly encourage skipping them and spending the time on more important areas instead.

As long as you run any app at all, it's always going to be immediately obvious that windowing is broken.

@Perksey Perksey changed the title [WIP] [4.0] Input and Windowing via GLFW [4.0] Input and Windowing via GLFW May 14, 2019
@Perksey Perksey changed the title [4.0] Input and Windowing via GLFW Input and Windowing via GLFW May 14, 2019
@varon
Copy link
Member

varon commented May 14, 2019

Going to start review on #871 first, as I'd like to see travis CI working for this if we can!

@devvoid
Copy link
Contributor

devvoid commented May 14, 2019

Sounds good, I still have another three days of finals before I'm free anyways

@devvoid
Copy link
Contributor

devvoid commented May 23, 2019

I got Travis to work again! There's still the multithreading issue (see Discord for more info), but otherwise, this should be 100% ready to go.

@jvbsl
Copy link
Contributor

jvbsl commented May 23, 2019

Wow can't believe that this is the fix. I explicitly added this line, because there were dotnet installation problems which were fixed by that(and it did work completely) :D

@devvoid
Copy link
Contributor

devvoid commented May 23, 2019

Ah, Travis, we love you, but you can be a royal pain...

@devvoid
Copy link
Contributor

devvoid commented May 23, 2019

Now that the threading thing is solved, unless there's anything else, this PR is now feature-complete.

That being said, I'd prefer to wait until after #908 is merged. There's a few places where float vectors are used, but integer vectors would be much cleaner.

However, this is ready for a review whenever a maintainer is free to do it.

@devvoid devvoid mentioned this pull request May 24, 2019
@varon
Copy link
Member

varon commented May 24, 2019

Let's aim to get those int vectors merged in if we can first!

@Perksey Perksey merged commit 9746538 into opentk:master May 26, 2019
OpenTK 4.0 automation moved this from In Progress to Done May 26, 2019
@Perksey Perksey mentioned this pull request May 26, 2019
@TheRealNOIG
Copy link

Does this GLFW GameWindow replace the original platform or is this a option to the original platform?

@varon
Copy link
Member

varon commented Apr 18, 2020

Completely replaces the native and SDL backends.

We're now only maintaining one, "high level" backend compared to four, which is a huge boost in our ability to ensure feature parity and reduce bugs across platforms.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
OpenTK 4.0
  
Done
Development

Successfully merging this pull request may close these issues.

Clipboard support
9 participants