-
Notifications
You must be signed in to change notification settings - Fork 13
Add support for MTL4 bindings #15
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
Add support for MTL4 bindings #15
Conversation
7323e16 to
9d9a86b
Compare
- Removed automatic scoping of unscoped methods since they might fall into wrong type with header updates; logging them instead to provide the handwritten bindings seems like more safe option - Fixed inheritance resolution (still not resolving the multiple inheritance, but that should not be anywhere in the project, i think) - Fixed collecting of selectors split over 2 lines - Fixed the get/set property pairs for is/set+is/setIs - Support for duplicated obsolete properties - Support for static properties
36428dc to
e9b6815
Compare
|
You can review it now. Not sure why the checks are failing, maybe because the PR is too big? :/ I tested the three examples and they work, but I might have introduced some other random error that is hard to find out based on the diffs. I also tested my small WIP project and that one works as well. Since I made the property logic a little bit more robust, there are some small BC breaks in the C# API (when something was previously a method and now is consistently a property). If you'd prefer the 2 codegen changes squashed together, as well as the 2 generated code ones, I can squash them into 1 codegen + 1 generated code. If you want any other changes to the PR, I'll fix it. P.S. I noticed some parts of the parsing that are kinda PITA, but this PR is bigger than I wanted it to be in the first place, so some places are kinda hacked-in. |
| { | ||
| public class PropertyInstance : IEquatable<PropertyInstance> | ||
| { | ||
| public readonly ClassInstance ContainingClass; |
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.
ClassInstance should be made nullable to account for those cases safely.
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 cherrypicked my local change from small cleanup I did along the way that splits the property instance into separate types for properties/function parameters/member variables of structs -- since those are logically different anyways, it doesn't IMO make sense to share the model representation type for it.
Feels better way to me to tackle the nullable thing, since the field is never needed anyways for the structs/function parameters.
|
I've noticed one significant failure in these generator changes in Metal 4 deprecates the function
[System.Obsolete]
public bool DepthWriteEnabled
{
get => ObjectiveCRuntime.bool_objc_msgSend(NativePtr, sel_isDepthWriteEnabled);
set => ObjectiveCRuntime.objc_msgSend(NativePtr, sel_setDepthWriteEnabled, value);
}
public bool IsDepthWriteEnabled
{
get => ObjectiveCRuntime.bool_objc_msgSend(NativePtr, sel_isDepthWriteEnabled);
set => ObjectiveCRuntime.objc_msgSend(NativePtr, sel_setDepthWriteEnabled, value);
}Both use the same new selector, and the logical name for this property in C# |
…unctionParameterInstance to reduce nulls
|
Actually, thinking about it |
|
It seems this error also occurs with |
|
There is a lot of the obsoletes. However, for the |
|
Ah, I didn't notice that, I guess this behaviour is acceptable then |
|
As for the multiple properties with some being obsolete - IMO it makes sense to expose either both (with old one marked as obsolete), or just the new ones for consistency naming, since new ones will have the new naming scheme -- but removing the old ones means extra BC breakes in C# usage, so it IMO makes sense to mimic the C++ binding behaviour. |
IsaacMarovitz
left a comment
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.
There are a few places where the generator is still not stable:
MTLCommandQueue.CommandBuffer()MTLCommandBuffer.RenderCommandEncoder()NSString.LengthOfBytes()
But I'll leave that to future PRs. Lgtm, nice work.
|
Yeah, I am aware of these, but these will be harder to fix. The resolution mechanism for selectors is not ideal, and basically the only fix IMO will be properly parsing the If I had to guess, these 3 are "known" (since they were handpicked & fixed in previous versions), but there will be a few more of these hidden errors in random methods that are just lost in the generated code. This is however something I want to tackle in a next PR - having the generator stable and matching all the selectors should be a priority. |
|
@pavelkouril Just realised that this actually is currently unusable. Thankfully in this case it's not |
|
@IsaacMarovitz I see. Sorry, I completely missed that. I knew the array APIs are an issue, but never occured to me that it's the only way to do stuff in MTL4. 🤦 I'll add support for the |
|
@pavelkouril No worries, I completely missed it too, I should've written the example before merging... Yeah, I'm happy with any such low-level collection being supported, |
Creating as a draft, since there are unresolved things at the moment.
Things I am aware of at the moment:
1/ Parser
[x] Error in parser that puts the
CreateDefaultSystemDevicein MTLArgumentDescriptor[x] Inheritance across multiple levels is not propagated (
Buffer->Resource->Allocation)[x] No parsing of the obsolete tags
2/ Code Emitting
[x] Missing properties in MTL4 files
[x] Wrong emitted types
[x] Static properties are emited as method instead of property (e.g.
NSBundle::allBundles())[x]
CAMetalLayer.PixelFormatemits only getter and uses the setter selector[x] Missing
MTL4BufferRange[x]
simd::float4x4emit3/ Examples
[x] Cannot be compiled due to the moving of
CreateDefaultSystemDevice[x] Examples crash upon start, probably due to some wrong selector being used now
Will tackle them one by one over the upcoming weeks, but it feels like some of these will require more substantial changes in the parsing/codegen (e.g. separation of the collecting and emitting passes - which would be a good idea anyways tbh).
(Will be squashed properly at the end before merging.)