-
Notifications
You must be signed in to change notification settings - Fork 475
Support @property getters for warp.struct #1155
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -460,6 +460,7 @@ def __init__(self, key: str, cls: type, module: warp._src.context.Module): | |
| self.cls = cls | ||
| self.module = module | ||
| self.vars: dict[str, Var] = {} | ||
| self.properties: dict[str, warp._src.context.Function] = {} | ||
|
|
||
| if isinstance(self.cls, Sequence): | ||
| raise RuntimeError("Warp structs must be defined as base classes") | ||
|
|
@@ -482,6 +483,14 @@ def __init__(self, key: str, cls: type, module: warp._src.context.Module): | |
| warp.init() | ||
| fields.append((label, var.type._type_)) | ||
|
|
||
| # Collect properties, but postpone Function creation until after native_name is set | ||
| property_members = [] | ||
| for name, item in inspect.getmembers(self.cls): | ||
| if isinstance(item, property): | ||
| if name in self.vars: | ||
| raise TypeError(f"Property '{name}' conflicts with field name in struct '{self.key}'") | ||
| property_members.append((name, item)) | ||
Ghelfi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| class StructType(ctypes.Structure): | ||
| # if struct is empty, add a dummy field to avoid launch errors on CPU device ("ffi_prep_cif failed") | ||
| _fields_ = fields or [("_dummy_", ctypes.c_byte)] | ||
|
|
@@ -502,12 +511,53 @@ class StructType(ctypes.Structure): | |
| if isinstance(type_hint, Struct): | ||
| ch.update(type_hint.hash) | ||
|
|
||
| self.hash = ch.digest() | ||
| # Hash property names (to ensure layout/identity stability if names change) | ||
| for name, _ in property_members: | ||
| ch.update(bytes(name, "utf-8")) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This only considers the name, but it should also include the function source. Otherwise modifying the getter won't trigger recompilation. |
||
|
|
||
| self.hash = ch.digest() | ||
| # generate unique identifier for structs in native code | ||
| hash_suffix = f"{self.hash.hex()[:8]}" | ||
| self.native_name = f"{self.key}_{hash_suffix}" | ||
|
|
||
| # Extract properties and create Functions | ||
| # self.native_name is now defined, so Function() can resolve 'self' type code. | ||
| for name, item in property_members: | ||
| # We currently support only getters | ||
| if item.fset is not None: | ||
| raise TypeError("Struct properties with setters are not supported") | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For now, only getters are supported. Setters should be doable too.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Without setters, this feature feels incomplete. |
||
| if item.fdel is not None: | ||
| raise TypeError("Struct properties with deleters are not supported") | ||
| getter = item.fget | ||
| # We need to add 'self' as the first argument, with the type of the struct itself. | ||
| # This allows overload resolution to match the struct instance to the 'self' argument. | ||
| if not hasattr(getter, "__annotations__"): | ||
| getter.__annotations__ = {} | ||
|
|
||
| # Find the name of the first argument (conventionally 'self') | ||
| argspec = get_full_arg_spec(getter) | ||
|
|
||
| if len(argspec.args) == 0: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does it make sense to allow more than one argument for the getter? How should that work? Would those need to be annotated by the user? I think it would currently fail during codegen without additional logic. Might be better to reject any function that doesn't have exactly one argument here. |
||
| raise TypeError(f"Struct property '{name}' must have at least one argument (self)") | ||
| self_arg = argspec.args[0] | ||
| getter.__annotations__[self_arg] = self | ||
|
|
||
| # Create the Warp Function. | ||
| # We pass 'func=getter' so that input_types and return_types are inferred. | ||
| # We set 'namespace=""' and a unique 'native_func' to generate a free function | ||
| # in C++ that takes the struct as the first argument (e.g., StructName_propName(struct_inst)). | ||
| p_func = warp._src.context.Function( | ||
| func=getter, | ||
| key=f"{self.key}.{name}", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The key shouldn't include the |
||
| namespace="", | ||
| module=module, | ||
| ) | ||
|
|
||
| # Ensure the C++ function name is unique and predictable | ||
| p_func.native_func = f"{self.native_name}_{name}" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be passed to the |
||
|
|
||
| self.properties[name] = p_func | ||
|
|
||
| # create default constructor (zero-initialize) | ||
| self.default_constructor = warp._src.context.Function( | ||
| func=None, | ||
|
|
@@ -2260,6 +2310,11 @@ def emit_Attribute(adj, node): | |
| else: | ||
| return adj.add_builtin_call("transform_get_rotation", [aggregate]) | ||
|
|
||
| elif isinstance(aggregate_type, Struct) and node.attr in aggregate_type.properties: | ||
| # property access | ||
| prop = aggregate_type.properties[node.attr] | ||
| return adj.add_call(prop, (aggregate,), {}, {}) | ||
|
|
||
| else: | ||
| attr_var = aggregate_type.vars[node.attr] | ||
|
|
||
|
|
||
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.
logic: No validation prevents properties from having the same name as struct fields, which could cause ambiguity since properties are checked before vars in attribute access (line 2309)