8000 Fix/imagerescale events by ResearchDaniel · Pull Request #1127 · inviwo/inviwo · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix/imagerescale events #1127

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

ResearchDaniel
Copy link
Contributor

Ensure that canvasSize of events matches the size of rescaled images. This behaviour matches that of the ViewManager

Copy link
Member
@petersteneteg petersteneteg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some initial thoughts. Can you explain I more detail how this is a problem? why do we even need to pass on the imageSize? if we do the translations correctly according to this, should we not end up with the original image size in that case? then the consumer could just use that size and the normalized coords or?
Given that it is needed, should the eventscaler not be in base or core?
Also does it handle cases where there aspect does not match? then one would need to adjust the positions to or? Should this functionality not go in the imageinport propagate event?

@petersteneteg
Copy link
Member

Is there a common base for events that hold a canvas size? If not why not add one? instead of repeating the various events...

@ResearchDaniel
Copy link
Contributor Author

Can you explain I more detail how this is a problem?

It is only a problem if one tries to calculate the integer pixel position of an event and expect it to match the ImageOutport of a processor.

Assuming that one has an image scaling processor that downsizes by 2 and then use an event that has propagated through that processor you could end up outside of the image outport in your processor “above” the scaling.
your processor
|
scaling
|
canvas

@ResearchDaniel
Copy link
Contributor Author

why do we even need to pass on the imageSize?

In case your processor does not have an image outport but need the event and wants to know the integer pixel position

@ResearchDaniel
Copy link
Contributor Author

Should this functionality not go in the imageinport propagate event?

Hmm, probably. In case the “original” canvas size is needed somewhere we could add that to the event

@ResearchDaniel
Copy link
Contributor Author

Is there a common base for events that hold a canvas size? If not why not add one?

we could move it to InteractionEvent. I considered it but never did it.

@petersteneteg
Copy link
Member

why do we even need to pass on the imageSize?

In case your processor does not have an image outport but need the event and wants to know the integer pixel position

When would you need the pixel position of the event and not have the image in question at hand?

@petersteneteg
Copy link
Member

Is there a common base for events that hold a canvas size? If not why not add one?

we could move it to InteractionEvent. I considered it but never did it.

Key events don't have a size I think. and those derive InteractionEvent, MouseInteractionEvent has the size, but TouchEvents inherits Interaction Event and also have a size. So Now would need an extra class between InteractionEvent and MouseInteractionEvent that also TOuchEvent can derive from

@ResearchDaniel
Copy link
Contributor Author

why do we even need to pass on the imageSize?

In case your processor does not have an image outport but need the event and wants to know the integer pixel position

When would you need the pixel position of the event and not have the image in question at hand?

cefEvent.x = static_cast<int>(e->x());

For convenience in many cases I guess. One could probably forward the size in most cases

@ResearchDaniel
Copy link
Contributor Author

Did a bit of research.
The ability to get canvasSize in an event is used in a few different places, e.g., BoxSelectionInteractionHandler, ViewManager, CefInteractionHandler. Those could probably be adjusted, but I think that it makes sense that one does not only have access to normalized coordinates in an event. This kind of leaves me with the conclusion that the canvasSize function should be there.

Moving this work into the ImageInport

We could override Inport::propagateEvent(Event* event, Outport* target) - create a copy of each event and set the canvas size to match that of the ImageInport.
Pros: No need to consider the canvas size for the programmer.
Cons: A copy of the event would need to be created for each ImageInport of each processor (probably no performance impact?). There would be more 'magic' with respect to event propagation (I currently don't know any scenario where this behaviour would be undesirable, but if there are it is very hard to undo the magic).

I'm not sure which solution is best here.

@petersteneteg
Copy link
Member

Agree I would probably be nice to have the size in the event. Did you think anything more on the aspect ratio mismatch, Im thinking about the cases when we add black padding to an image for example. Also does this interact with the image port resize behavior, OutpotDetermainsSize and so on?

@ResearchDaniel
Copy link
Contributor Author

The ratio-matching cases will probably need to do something similar to the ViewManager. A bit more complicated than the problem I had in mind...

Also does this interact with the image port resize behavior, OutpotDetermainsSize and so on

It is based on the actual size of the inport so I think that should not be a problem. I tried a couple of different settings in the ImageResampler manually and it seemed to work as intended. One would have to take care that the EventScaler has the correct size set. We could use a lambda here to avoid duplicating state.

…ort dimensions does not match

BaseGL: ImageResample fix for empty inport on resize

Jenkins: Format fixes
8000 @petersteneteg petersteneteg force-pushed the fix/imagerescale-events branch from dd66037 to 8cad598 Compare February 25, 2022 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
0