beginning base logic for interactivity impl #61
Conversation
|
maybe an even simpler way to implement this:
def link(
self,
event: str,
target: Graphic,
feature: str,
new_data: Any,
indices_mapper: callable = None
):
# event picks subset of self.indices -> indices_mapper() -> indices_subset -> target._set_feature(new_data, indices_subset) |
|
Made the changes that you requested, I'm sure there are further changes needed to be made going to move on to linecollection and try to get that up and running! |
|
sorry for all of the commits, git was not my friend for a moment but everything should be fine now I hope |
|
So, I got it to work with line collection things are not perfect by any means but got a nice dopamine rush...hopefully indicative of things moving in the right direction |
There was a problem hiding this comment.
great work! you can continue playing with it, once #78 is ready (very soon!) it'll be much easier to do the rest
| if event_type in self.registered_callbacks.keys(): | ||
| self.registered_callbacks[event_type].append( | ||
| CallbackData(target=target, feature=feature, new_data=new_data, old_data=getattr(target, feature).copy())) | ||
| CallbackData(target=target, feature=feature, new_data=new_data, old_data=old_data, indices_mapper=indices_mapper)) |
There was a problem hiding this comment.
old_data shouldn't be in CallbackData, old_data should be managed by the target graphic event time it handles an event. When the target graphic handles an event, it should have a copy of the feature data at the indices that are being modified and reset the feature at those indices, and then set the feature w.r.t. to the new CallbackData.
To elaborate:
-
The target graphic keeps a private dict of
(features, orig_vals)that were modified at certain indices from the previous callback, something in the form{"feature": (indices, orig_vals_at_indices)}, you could call itself._previous_modified. And with the newGraphicFeatureclass from indexable graphic attributes #78 we won't even need a dict for it, we can just add_modified_indicesand_orig_vals_at_modified_indicesas private attributes toGraphicFeature😄 . -
In
_set_feature()it first uses the information from above to reset the feature at those indices to their original values
There was a problem hiding this comment.
with #78 it might be possible for us to just pass the GraphicFeature instance directly instead of the string name when calling link, for example:
graphic.link(event_type, target=some_graphic, feature=some_graphic.color...
There was a problem hiding this comment.
If info for resetting feature is moved to _set_feature() graphic, then we will no longer need method for _reset_feature()?
There was a problem hiding this comment.
we should still have a _reset_feature() to separate functionality
| indices = target_info.indices_mapper(target=target_info.target, indices=click_info) | ||
| else: | ||
| indices = None | ||
| # reset feature of target using stored old data |
There was a problem hiding this comment.
ah I see you've done it like this, but it's better for the target graphic to handle old data because we want the "feature resetting at previously modified indices" to happen regardless of the event type. For example, if we've registered different types of callbacks to keyboard events, and different mouse events, we want the same reset to happen regardless of what previous event.type triggered the previous feature data to change.
There was a problem hiding this comment.
I agree! Just wasn't sure at the time the best way to implement, but now that graphic features exist it should be a lot easier to store old data!
| update_func = getattr(self, f"update_{feature}") | ||
| self.collection[indices].update_func(new_data) | ||
| if feature in self.features: | ||
| update_func = getattr(self.data[indices], f"update_{feature}") |
There was a problem hiding this comment.
the way it's written it'll only work with single integer indices, we can chat about it
There was a problem hiding this comment.
once we implement this it'll be much easier #76 (comment)
it would become getattr(self, feature)[indices] = new_data
|
|
||
| @property | ||
| def features(self) -> List[str]: | ||
| pass | ||
| return ["colors", "data"] | ||
|
|
||
| def _set_feature(self, feature: str, new_data: Any, indices: Any): |
There was a problem hiding this comment.
I think that for the _set_feature method in each Graphic or GraphicCollection we can replace Any for the indices to the indices that are appropriate for that graphic type.
| for target_info in self.registered_callbacks[event.type]: | ||
| # need to map the indices to the target using indices_mapper | ||
| if target_info.indices_mapper is not None: | ||
| indices = target_info.indices_mapper(target=target_info.target, indices=click_info) |
There was a problem hiding this comment.
an indices_mapper function should be given source and target, something like this:
indices_mapper(source=self, target=target_info.target, event_indices=click_info
And the user can write their function to take in all 3 and do whatever they want.
There was a problem hiding this comment.
yes, I assumed that some general form would need to be established
|
Now that #78 brings events to def link(self, event_type: str, target: Any, ...):
pygfx_events = ["click", ...]
graphic_feature_events = ["colors", "data", "present", ...]
if event_type in pygfx_events:
self.world_object.add_event_handler(...)
elif event_type in graphic_feature_events:
feature = getattr(self, event_type)
feature.add_event_handler(...) |
|
should probably also generalize the |
starting #39