-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[PAID BOUNTY] A shared base for public and console repos #8309
base: develop
Are you sure you want to change the base?
Conversation
public int MaxVertexBufferSlots; | ||
} | ||
|
||
internal static unsafe partial class MGG |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to wart these with something that works for us in C# and in C++ (don't want conflicts with other native libs).
So i'm thinking:
- MGG for Graphics.
- MGA for Audio.
- MGI for Input.
- MGF for Framework?
Thoughts?
#include "csharp_structs.h" | ||
|
||
|
||
struct MGG_GraphicsDevice; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea here is the C++ implementation will define the internals of these pre-declared structures.
For example one for DX could look like:
struct MGG_GraphicsDevice
{
ID3D12Device* _device;
ID3D12GraphicsCommandList* _commandList;
ID3D12CommandQueue* _commandQueue;
// etc!
}
This would be how native platforms pass along its data across pinvokes.
The C# just passes pointers and the C++ is responsible for allocation and freeing of these types.
src/monogame/include/csharp_enums.h
Outdated
|
||
#include "csharp_common.h" | ||
|
||
enum CSSurfaceFormat : csint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here... MGSurfaceFormat
instead of CSSurfaceFormat
?
// | ||
// So skip these. | ||
// | ||
if (type.GetCustomAttribute<IsReadOnlyAttribute>() == null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels sloppy depending on the readonly
flag to identify our handle types.
Any better ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My idea was to skip the types which names end in Ptr
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps a fully qualified attribute? Say annotate with [MGHandleAttribute]
which works well with the reflection here as you have the type
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep this worked better:
[MGHandle] internal readonly struct MGG_GraphicsSystem { }
Thanks @Mindfulplays
|
||
MGG_GraphicsDevice* MGG_GraphicsDevice_Create() | ||
{ | ||
auto device = new MGG_GraphicsDevice(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@harry-cpp what do you think of having a no op backend that has empty implementations?
Seems it could be good for some cases.
37fb5ff
to
5f845ec
Compare
@harry-cpp what do i need to do for this? Is it just building? Or is it also a project generator for C++? I had assumed we would maintain C++ projects manually on a per-platform basis. |
/// <summary> | ||
/// MonoGame image native calls. | ||
/// </summary> | ||
internal static unsafe partial class MGI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should ditch StbSharp for native stb directly since we have a native library.
This API exposes reading images (jpg/png) from memory as well as writing jpg and png.
We could implement more here in the future.
I assume we'll have one Windows C++ project for a Or should we plan on multiple C++ Windows projects? I assume we won't support runtime swapping of Vulkan or DX12? Especially since we pre-build the effects for the platform. (IMO this is a gimmick and not actually useful in shipping games) So we will end up with Also i assume WindowsGL is going away. |
I don't have a strong opinion on this. Whatever seems smooth enough in regard to Linux, macOS, and consoles moving to that too.
It doesn't feel useful to me. Not on the C++ at least. Worst case we can manage that on the C# side of things with a simple switch loading either native dll at launch. |
It should:
CAKE is just building the projects. But. We should use some cross platform solution for building the projects as VS projects are Windows only. Some of them even support generating a VS project for you to work in. Good options are meson and cmake. |
@@ -74,11 +74,7 @@ protected Texture3D (GraphicsDevice graphicsDevice, int width, int height, int d | |||
{ | |||
ValidateParams(level, left, top, right, bottom, front, back, data, startIndex, elementCount); | |||
|
|||
var width = right - left; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figured i would fix bad platform apis when i could.
Here it was passing left, top, right, bottom, front, back.... AND width, height, depth.
So i removed the extra data passing that not all platforms need.
73e2947
to
948100b
Compare
|
||
internal SamplerStateCollection(GraphicsDevice device, int maxSamplers, bool applyToVertexStage) | ||
internal SamplerStateCollection(GraphicsDevice device, int maxSamplers, ShaderStage stage) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This felt like a better design moving forward.
|
||
#pragma once | ||
|
||
#include "csharp_common.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The prefix csharp_
feels wrong here. Better ideas? gen_
or api_
or something?
@@ -0,0 +1,19 @@ | |||
@echo off |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decided it was best to keep the generated binary effect files in per-platform folders instead of all in a common place.
This seems like a good direction considering console support.
// Or do we make the native code transform the string? | ||
// | ||
var absolutePath = Path.Combine(Location, safeName); | ||
return File.OpenRead(absolutePath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will be moving the file path fixup to native code.
@@ -80,7 +80,7 @@ struct VSInputNmTxWeights | |||
float4 Position : POSITION0; | |||
float3 Normal : NORMAL0; | |||
float2 TexCoord : TEXCOORD0; | |||
uint4 Indices : BLENDINDICES0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works around an issue DXC/Glsl where unsigned int 4 didn't compile. This shouldn't matter as you never have huge numbers for blend indices.
@@ -20,7 +20,7 @@ ColorPair ComputeLights(float3 eyeVector, float3 worldNormal, uniform int numLig | |||
float3x3 lightSpecular = 0; | |||
float3x3 halfVectors = 0; | |||
|
|||
[unroll] | |||
UNROLL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using macros here so it can be redefined per-platform.
/// <see cref="InputLayoutCache"/>. | ||
/// </summary> | ||
/// <returns>The <see cref="ImmutableVertexInputLayout"/>.</returns> | ||
internal ImmutableVertexInputLayout ToImmutable() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved this from DirectX specific code to be shared between platforms.
private int _dirty; | ||
|
||
internal TextureCollection(GraphicsDevice graphicsDevice, int maxTextures, bool applyToVertexStage) | ||
internal TextureCollection(GraphicsDevice graphicsDevice, int maxTextures, ShaderStage stage) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a better approach considering we may eventually have more shader stages.
|
||
LPCSTR id = nullptr; | ||
|
||
if (strcmp(name, "AlphaTestEffect") == 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we consider making an enum for the stock effects instead of passing around string names here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
src/monogame/vulkan/MGG_Vulkan.cpp
Outdated
|
||
static MGG_System* System_Get() | ||
{ | ||
if (mgg_system != nullptr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm debating if we need some object like MGG_System
exposed at the C# level to manage graphics state before a graphics device is created... like initializing the Vulkan API (or initializing DX12).
Or should we let the native code manage this internally?
src/monogame/vulkan/MGG_Vulkan.cpp
Outdated
VkApplicationInfo app_info = { VK_STRUCTURE_TYPE_APPLICATION_INFO }; | ||
app_info.pNext = nullptr; | ||
app_info.applicationVersion = VK_MAKE_VERSION(1, 0, 0); | ||
app_info.pApplicationName = "None"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we pass down an application name... i assume this is used by drivers and utilities for optimizing/tweaking settings for specific games.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, vulkan has fields to pass both game name and engine name so the driver can optimise it if it so chooses.
|
||
#ifdef _WIN32 | ||
|
||
HMONITOR primaryMonitor = MonitorFromPoint(POINT{ 0, 0 }, MONITOR_DEFAULTTOPRIMARY); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably all wrong and needs to be rewritten, but it works for this first pass at Vulkan.
src/monogame/include/stl_common.h
Outdated
|
||
|
||
template <class T> | ||
void remove_by_value(std::vector<T>& vector, const T& element) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now i'm depending on STL for vectors, string, queue, maps, etc.
I have seen differences in functionality and performance across platforms over the years.
I wonder if eventually we want our own small common set of native helper collections to use across our native backends?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should only implement our own if we come across an issue with performance.
@@ -34,13 +34,13 @@ public abstract class GameWindow | |||
/// </summary> | |||
public virtual bool AllowAltF4 { get { return _allowAltF4; } set { _allowAltF4 = value; } } | |||
|
|||
#if (WINDOWS && !WINDOWS_UAP) || DESKTOPGL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking we should just expose this on all platforms and let it be ignored on platforms that cannot use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed.
/// <summary> | ||
/// All desktop versions using Vulkan. | ||
/// </summary> | ||
DesktopVK, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mrhelmut @harry-cpp this is the plan correct?
// TODO: Why did this start with SDL_WINDOW_FULLSCREEN_DESKTOP in the old C# SDL? | ||
// We should write coments to document odd behaviors like this. | ||
|
||
Uint32 flags = SDL_WINDOW_HIDDEN;// | SDL_WINDOW_FULLSCREEN_DESKTOP; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@harry-cpp do you remember why the C# SDL implementation was making this a SDL_WINDOW_FULLSCREEN_DESKTOP by default?
For visibility here is the initial rendering under SDL2 + Vulkan on the new native backend: ice_video_20240616-215942.mp4Has bugs, but we have progress too. |
@@ -1,4 +1,4 @@ | |||
<Project Sdk="Microsoft.NET.Sdk"> | |||
<Project Sdk="Microsoft.NET.Sdk"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder why git is showing a diff here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A whitespace change of some sort. It happens.
platform->exit = true; | ||
break; | ||
|
||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a reminder to not leave so much comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WIP... i expect these will be restored in the next week.
if (_system == null) | ||
{ | ||
Environment.SetEnvironmentVariable("VK_LOADER_DEBUG", "all"); | ||
Environment.SetEnvironmentVariable("VK_LAYER_PATH", "C:\\VulkanSDK\\1.3.283.0\\Bin"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks very os specific
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I already removed it. It was some hacked code when I was trying to fix Vulkan validation.
This is a WIP on the shared native base for future platform work.
See #8242
Looking for feedback on the structure of things before i finish the implementation and finalize it.
C# to C++ wrapper generator.C# native wrapper.Windows VK setup.