Skip to content
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

fix mouse position for 3D canvas #20442

Closed
wants to merge 0 commits into from
Closed

Conversation

pmp-p
Copy link
Contributor

@pmp-p pmp-p commented Oct 11, 2023

does not work for 2D but i don't know why, but leaving 2D as it is, fixing 2D is much more complicated anyway ( transformation matrix could apply see https://stackoverflow.com/questions/17130395/real-mouse-position-in-canvas for more )

@pmp-p pmp-p changed the title fix mouse position for 3D canvas (draft) fix mouse position for 3D canvas Oct 11, 2023
Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

I don't know about about these APIs to know if this the right direction, but will this not break existing users of the API? Can you describe a little more the issue that this is addressing? Perhaps open a bug?

Can we find a way to test this?

src/library_html5.js Outdated Show resolved Hide resolved
src/library_html5.js Outdated Show resolved Hide resolved
@pmp-p pmp-p changed the title (draft) fix mouse position for 3D canvas fix mouse position for 3D canvas Oct 11, 2023
@pmp-p
Copy link
Contributor Author

pmp-p commented Oct 11, 2023

now canvas 2D users are not affected. non scaled canvas 3D users either.

The only users of scaled 3D canvas i know are Harfang3D and Panda3D, and mouse coordinates are wrong in both their GUI ( Harfang3D imgui ) and Panda3D directgui.

I actually test for them. I don't know of anything production ready using that scheme ( canvas css scaling + 3D gui ) yet. Do you ?

@sbc100
Copy link
Collaborator

sbc100 commented Oct 11, 2023

now canvas 2D users are not affected. non scaled canvas 3D users either.

The only users of scaled 3D canvas i know are Harfang3D and Panda3D, and mouse coordinates are wrong in both their GUI ( Hargang imgui ) and Panda3D directgui.

I actually test for them. I don't know of anything production ready using that scheme ( canvas css scaling + 3D gui ) yet. Do you ?

No, but I don't know really know this part of the codebase well, or who uses it.

@juj might have more insight.

src/library_html5.js Outdated Show resolved Hide resolved
pmp-p added a commit to pygame-web/python-wasm-sdk that referenced this pull request Oct 12, 2023
@juj
Copy link
Collaborator

juj commented Oct 16, 2023

It looks like this PR is attempting to translate CSS coordinates into canvas device pixel coordinates.

There are a few issues with this change:
a) The original intent of this API is to report the data from the input events directly, with zero (or at most minimal) processing generated by Emscripten, that the users could as well do themselves. This way the code remains more like a bridge to existing JavaScript APIs where users can use existing MDN documentation to reason about them, and less of a "framework infrastructure" that Emscripten would be building on top of the web.

This means that it is intentional that the coordinates that are reported by these events, are the CSS coordinates from the web events, untransformed. So it will be the role of the user to choose to either do their computations in CSS coordinates, or to map the coordinates over to device pixel coordinates, depending on their app logic.

b) Suddenly switching to doing this transform in library_html5.js would break all users who rely on these fields. If existing users had their app calculate coordinate computations in CSS pixels, this would cause a breakage for them. Likewise, even if an existing user had coordinate calculation as WebGL render target device pixels, this change would also break them, since the scaling would after this PR be done twice for them.

c) calling canvas.getContext('2d') in library_html5.js would cause issues. This is because the .getContext() function does not only inquire if an existing 2d context is set, but it allocates a new one if not.

A canvas can have only one type of context (2d, webgl or webgl2) associated with it, and the first one getContext() is called with will be the chose one.

So with this kind of change, if a user would interact over the canvas with mouse before the Emscripten user code was able to initialize a webgl context, this code would make the canvas get stuck with a 2d context, preventing the application from being able to allocate a webgl context at all.

I recommend reaching out to Harfang3D and Panda3D to patch their event handler code to implement transformations from CSS space to WebGL canvas client space in the callbacks. Doing this in WebAssembly side has an added benefit of providing smaller code size as well.

@pmp-p
Copy link
Contributor Author

pmp-p commented Oct 16, 2023

Thanks for the insight i did not think that change would open so much perspectives.

a) I somehow disagree : mouse/keyboard/canvas2D/3D/webusb ie anything hardware is unrelated to javascript world : from wasm point of view emscripten is an OS layer, and so it should do the (good) math. What's the point of everyone bringing its own OS layer and yet others API again ? Personnaly i want to read Emscripten API not MDN. If i'm caught reading MDN it is probably that i'm trying to run wasi binaries on web which is probably a bad idea :D

b)
That's why i was asking if you know anyone - named - using that. Because so far i'll assume that for the only one reported using that : mouse reporting is broken (even from SDL2 point of view i think). I guess a new entry in API for real buffer pixels should be added with some thinking on names. The actual name is confusing it should be more something like viewportX instead of canvasX.

Me only doing very basic 2D/3D and in my head mouse is from -1.0 to +1.0 both axis in camera portal so i don't really understand your point.
Pinging @rdb @astrofra about that, if it's trivial they'll explain to me directly.

about c)

A canvas can have only one type of context (2d, webgl or webgl2) associated with it, and the first one getContext() is called with will be the chose one.

in which conditions would someone call mouse position on a unassigned context and what then should be the result ? mind that 2D canvas is always reporting pixel values making floating state inconsistent.

I recommend reaching out to Harfang3D and Panda3D to patch their event handler code to implement transformations from CSS space to WebGL canvas client space in the callbacks. Doing this in WebAssembly side has an added benefit of providing smaller code size as well.

it seems to me that has a direct impact on performance as it need complex caching with handling resize/rotation events which may not even fit in some games loops.

Is not the primary goal of emsdk to port codebases to web with less possible modifications, not porting web noise everywhere into existing codebases ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants