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
draft Metal implementation #1185
Conversation
@@ -115,6 +115,7 @@ class GlslGenerator constructor( | |||
|
|||
//println("types.funcRefs=${types.funcRefs}") | |||
val customFuncs = funcs.filter { it.ref.name in types.funcRefs }.reversed().distinctBy { it.ref.name } | |||
//TODO: is it relevant to visit mainFunc for each existing function ? | |||
for (func in funcs) types.visit(mainFunc) |
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.
@soywiz found this on the gl implementation, not sure to understand the point.
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 a typo
@soywiz one question so far, Metal shader must use structure as input/output of vertex and fragment functions, but the current implementation of |
gradle.properties
Outdated
enableKotlinMobile=true | ||
enableKotlinAndroid=true | ||
enableKotlinAndroid=false |
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.
to revert later
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.
did you have any kind of errors because of this? Would be nice to figure out the root cause so other people can benefit from 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.
gradle.properties
Outdated
@@ -62,7 +62,7 @@ kotlin.native.ignoreDisabledTargets=true | |||
#kotlin.mpp.androidSourceSetLayoutVersion=2 | |||
kotlin.mpp.androidSourceSetLayoutVersion1.nowarn=true | |||
|
|||
#org.gradle.caching=true | |||
org.gradle.caching=true |
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.
to revert later
Thanks for the PR/proposal. I'm open to the suggestions your propose, but I believe we should handle them independently and separately to this PR, to avoid the complexity of handling everything at once. Let's use the forum for example: Now let's focus in this PR about the metal shader generation. I believe since it can be added to the repository without affecting any production feature, it could be merged even if incomplete. Just an initial step, then a test Makes sense to you? |
I have created several discussion threads for the major changes you proposed. Can we discuss them now? Some of them will likely affect compatibility, so now that we are still in 4.0 alphas we can consider it. After 4.0 is published, those proposals would have to wait for the next major release:
If I missed something, please create a new discussion in the Ideas and discussions section. |
@soywiz small update AG and shader generation support basic elements such as vertex, position, and color. My next step are :
As there is no such things as small victory : Using the code
|
Nice! I have a pending PR: that will produce uniforms in a buffer you will be able to directly upload to the GPU and use indices for them. But it is stil a WIP |
remove kotest fix tests
@ygdrasil-io Okay so I believe we should merge something already, then add a feature flag to disable it for now so you can continue working on it but still refactors and stuff are having this into account. we can add a feature flag here by using environment variables:
then once ready we can add DISABLE_METAL as long as ir compiles, and metal failing tests are disabled and opengl backend is working, it should be fine merging it the only thing is please, even if in other packages, can you put stuff in korgw? I still have to check how to split in modules, because that would create extra artifacts of name I dont know and every extra artifact has to download like 8 files per target and that adds up like a lot of time for new users to get started. I have to experiment and check how to do it |
@soywiz Anything else to change ? |
Nope. Let's merge it and iterate |
Not yet an official PR, but more a base of discussion to a proper metal implementation.
This is following the discussion here #1155.
Be open minded, everything here is only a proposal.
Not sure I have enough knowledge in graphics pipeline to do a complete implementation, but at first I will try to cover shader generation and compilation, then a basic rendering on a MTKView.
From my point of view, Korge code is not necessarily easy to parse so I tried to improve readability with digression regarding the actual code. Feel free to argue and I will update the code according to this.
2-1. Projects namings and organizations :
I think that using names like korgw, korio, etc.. make it difficult to newcomer to understand the projects organization, so, i created new sub project like this
2-2. Misc