Start of windowing implementation #1007
Conversation
Implements "Stub" windowing API, the framework is there, but the meat and potatoes are not currently implemented
| bool IsCloseRequested { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// Gets whether the window is focused or not. |
There was a problem hiding this comment.
I don't know, all of the interfaces are pretty much pulled from the proposal directly, if you have issues with it, it might be best to talk about overall proposal changes.
| /// <summary> | ||
| /// Gets the distances in screen coordinates from the edges of the content area to the corresponding edges of | ||
| /// the full window. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// Because these are distances and not coordinates, they are always zero or positive. | ||
| /// </remarks> | ||
| /// <seealso cref="WindowExtensions.GetFullSize"/> | ||
| Rectangle<int> BorderSize { get; } |
There was a problem hiding this comment.
It's unclear to me what the docs are saying here. We don't use the concept of a content area anywhere else.
Is there a different way to word this to make it easier to understand?
There was a problem hiding this comment.
I have no idea what it is.
There was a problem hiding this comment.
The content area describes the space a window can actually take up. So on a 2160p monitor, it will be 3840 x (2160 - <size of taskbar>) if that makes any sense.
| /// <summary> | ||
| /// Raised when the window has been requested to close. | ||
| /// </summary> | ||
| event Action CloseRequested; |
There was a problem hiding this comment.
Is the corresponding boolean property set before or after?
(add to remarks)
In line with other properties, there should be a delegate providing the new value and the old value should be found in the property.
There was a problem hiding this comment.
Set before (the user can cancel closure by setting it back to false), agree needs remarks (in proposal)
| /// <summary> | ||
| /// Gets or sets whether the window waits for an event to be posted before existing <see cref="DoEvents" />. | ||
| /// </summary> | ||
| bool IsEventDriven { get; set; } |
There was a problem hiding this comment.
Does the proposal go into details about this? Seems like a relic of 2.x that's not really needed?
There was a problem hiding this comment.
Given the update loop is very similar to 2.X this will still be needed for applications (e.g. UI) that do not regularly need to render (i.e. only when an animation changes or some other event happens such as the user moving their mouse or the window etc)
| /// <summary> | ||
| /// The position of the window. If set to -1, use the backend default. | ||
| /// </summary> |
There was a problem hiding this comment.
Add a <see... To where this backend default can be found.
Also why did we make the user figure this out?
Additionally, can't a window actually be outside the screen (negative coordinates), at least with whatever anchor point is used here?
There was a problem hiding this comment.
Someone will need to pick apart the logic I wrote in 2.X to determine whether that's possible (or just experiment separately)
if you want to find the code you can search for marks] and you'll find it.
There was a problem hiding this comment.
I don't know if that is something we even know AOT, or if we have a type that exposes it at all, or if GLFW and other APIs expose such a thing in the first place.
|
|
||
| namespace Silk.NET.Windowing | ||
| { | ||
| public readonly struct VideoMode |
|
|
||
| namespace Silk.NET.Windowing.Implementations.GLFW; | ||
|
|
||
| public class GLFWSurface : IGLTransparentFramebuffer, IGlesDesktopSurface, IVkDesktopSurface, IGLDesktopSurface |
There was a problem hiding this comment.
This doesn't seem like it'll work 🤔 we will see when implementing I guess
There was a problem hiding this comment.
Why would it not work?
| <PropertyGroup> | ||
| <TargetFrameworks>$(SilkTargetFramework)</TargetFrameworks> | ||
| <Nullable>enable</Nullable> | ||
| <AllowUnsafeBlocks>true</AllowUnsafeBlocks> | ||
| <GenerateDocumentationFile>true</GenerateDocumentationFile> | ||
| <WarningsAsErrors>true</WarningsAsErrors> | ||
| <LangVersion>9.0</LangVersion> | ||
|
|
||
| <EnforceCodeStyleInBuild>true</EnforceCodeStyleInBuild> | ||
| </PropertyGroup> |
There was a problem hiding this comment.
Todo: verify with rest of project setup
There was a problem hiding this comment.
This was based on silk.net.Maths
There was a problem hiding this comment.
Yeah, but the 3.0 setup is quite different. I'll review it when everything else is done, just left this here to remind myself
There was a problem hiding this comment.
I mean it was pulled from the Silk.net.Maths implementation currently sitting in the directory right beside it, I figured at worst it would not completely break anything
|
I can answer any outstanding proposal wording questions in a bit, really annoyed this didn’t come up in #539 though. |
|
Yeah me too - We've been less then thorough with that review. I guess I was just so into the idea cause we came up with it only few days before, and didn't see any problems with it. |
|
Can you propose a change to the proposal itself? It's unfair to expect an external contributor to answer to this. |
|
I don't really expect any contributor to answer this, I just figured tracking it here associated directly with the code seems easier. If you want I'll delete the proposal and re-submit it, and attach the comments there. |
|
I was more suggesting propose changes to the proposal in response to your comments yourself, probably the most productive :) again I can help respond in a bit. |
|
Ah wups, yeah will do that once I'm done with my current work item. Just wanted to get my review out there, and will resolve any outstanding things once I get to windowing. |
Does this mean I other than some the slight changes I am stuck? |
|
Not necessarily, your free to put in any work you want to, but no one expects you to do any of the proposal work. |
Implementing test for Version32, moving Version32 to Core, removing implementations from Interfaces among other stuff I probably forgot.
Redhacker1
force-pushed
the
develop/3.0
branch
from
0e95025 to
f02b74f
Compare
…into develop/3.0
For some reason my branch is not picking it up
|
This has an outstanding action on @HurricanKai to change the proposals, but there is little work being done here at this time. Should we merge just so we have something in the repo as this does technically implement the approved API as it currently stands? |
|
I think this activity has proven that we're not necessarily ready to work on this right now. Opinions for closing for now? |
|
Doesn't seem like this will be implemented anytime soon, and there's lots of conflicts. |
4 participants
Implements "Stub" windowing API, the framework is there, but the meat and potatoes are not currently implemented
Summary of the PR
Essentially adds the interfaces as defined in the Windowing(3.0) proposal