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

Automatically keep track of transformations #7342

Closed
2 of 17 tasks
RandomGamingDev opened this issue Oct 31, 2024 · 13 comments
Closed
2 of 17 tasks

Automatically keep track of transformations #7342

RandomGamingDev opened this issue Oct 31, 2024 · 13 comments

Comments

@RandomGamingDev
Copy link
Contributor

Increasing access

This would mean that transformations can be fully managed by p5.js, which not only means increased ease of control for the user, but also not having to have 2 sets of data for the same thing, one on p5.js's side and one on the user's side.

Most appropriate sub-area of p5.js?

  • Accessibility
  • Color
  • Core/Environment/Rendering
  • Data
  • DOM
  • Events
  • Image
  • IO
  • Math
  • Typography
  • Utilities
  • WebGL
  • Build process
  • Unit testing
  • Internationalization
  • Friendly errors
  • Other (specify if possible)

Feature request details

While this is already possible in P2D mode with drawingContext.getTransform(); it doesn't apply to WebGL which doesn't use CanvasRenderingContext2D. Also, the transform returned with drawingContext.getTransform(); is a transformation matrix, which many people won't know how to use.

The way this would be done is through an official p5.js functions that return the transformation matrix, transform or untransform points, or return attributes that are decided as essential (the rest can be taken from the transformation matrix).

@davepagurek
Copy link
Contributor

Related, this PR #7113 is adding support for worldToScreen -- I think in a similar format, screenToWorld would be useful. Direct access to matrices would help for more advanced usage but, as you mention, not every user will be familiar with how to use those. It'd be useful to have a stable API for that for library authors though. For WebGL, it's a little less clear what should be included in the matrix (just model? model + view?) so it might be worth adding some optional parameters like { includeCamera: true } to whichever methods we add.

@RandomGamingDev
Copy link
Contributor Author

For WebGL I'd say to go with the multiplied result of the model, view, and projection matrices that are typically used for vertex shader transforms.

Also, although the people who use the matrix directly would be more technically inclined, it'd still be best to make things consistent across both the P2D and WebGL renderers by doing the same for P2D, both for the functions and returned matrix except with locked values for things like the z axis which are technically just constant for P2D.

@Garima3110
Copy link
Member

Garima3110 commented Nov 2, 2024

I've implemented a screenToWorld() function that converts screen coordinates back to world coordinates for both 2D and WebGL contexts. The function uses inverse transform matrices to revert back to world space, making it versatile across 2D and 3D contexts.

p5.prototype.screenToWorld = function(screenPosition) {
  const renderer = this._renderer;
  if (renderer.drawingContext instanceof CanvasRenderingContext2D) {
    // Handle 2D context
    const inverseTransformMatrix = new DOMMatrix()
      .scale(renderer._pInst.pixelDensity())
      .multiply(renderer.drawingContext.getTransform().inverse());
    const worldCoordinates = inverseTransformMatrix.transformPoint(
      new DOMPoint(screenPosition.x, screenPosition.y)
    );
    return new p5.Vector(worldCoordinates.x, worldCoordinates.y);
  } else {
    // Handle WebGL context (3D)
    const normalizedDeviceCoordinates = new p5.Vector(
      (screenPosition.x / this.width) * 2 - 1,
      (screenPosition.y / this.height) * -2 + 1,
      screenPosition.z || 0
    );

    const inversePMatrix = renderer.states.uPMatrix.invert();
    const inverseMVMatrix = renderer.calculateCombinedMatrix().invert();

    const cameraCoordinates = inversePMatrix.multiplyPoint(normalizedDeviceCoordinates);
    const worldCoordinates = inverseMVMatrix.multiplyPoint(cameraCoordinates);

    return new p5.Vector(worldCoordinates.x, worldCoordinates.y, worldCoordinates.z);
  }
};

The calculateCombinedMatrix() method that I wrote for the worldToScreen() method here :

calculateCombinedMatrix() {

could also be enhanced with an optional camera transformation parameter as suggested by @davepagurek .
Let me know if I can proceed with this and submit a PR for the issue, @RandomGamingDev .
Thankyou!

@RandomGamingDev
Copy link
Contributor Author

Let me know if I can proceed with this and submit a PR for the issue, @RandomGamingDev .
Thankyou!

It looks great! You should proceed with the issue.

After that, all that'll be left is returning attributes individually.

@Garima3110
Copy link
Member

It looks great! You should proceed with the issue.

Thanks, I'll submit a PR soon!

@Garima3110
Copy link
Member

I apologize for the delay in working on this issue due to some health issue, just in my recovery phase will get back to working on this super soon, till then If anyone wants to work on this, you can, I would try giving my reviews for that too.
Thankyou!

@bojidar-bg
Copy link

@Garima3110 Hoping you have a speedy recovery! I added (and slightly corrected) the function you gave above in #7561, in case you'd like to review it. (:

@davepagurek
Copy link
Contributor

@limzykenneth @ksen0 while we have worldToScreen and screenToWorld now, I've noticed a number of WebGL libraries broke after 1.9.2 when we separated _renderer.uMVMatrix internally into a model and view matrix instead of always using the combined one (e.g. p5.filterRenderer, p5.brush, p5.asciify.) Some of them fixed themselves by using the new matrices, but since 2.0 currently does not have access to _renderer and those have moved into states so they will break again as it stands. It feels like this is very common at least for WebGL -- do you think it makes sense either making official global accessors for these, or at least making the renderer officially accessible again and putting methods in at least the WebGL renderer to access these that we won't change?

@limzykenneth
Copy link
Member

@davepagurek The renderer should still be accessible through this._renderer in the context where this is the p5 instance, ie. p5.prototype methods and lifecycle hooks functions in the 2.0 library syntax. _renderer is not attached globally in global mode and I think it's better to not rely on the presence of a global value since it would need different codebase in the addon to handle both global mode and instance mode.

The states object can be accessed through this._renderer.states and should be relatively stable for now. We can have a think about more standardization around accessing internal values by addons to ensure things don't change in terms of interface while still providing flexibility.

Does accessing this._renderer solves the issue here?

@davepagurek
Copy link
Contributor

Accessing this._renderer definitely helps, I think the question then becomes whether we should tell addons to access the matrices in states now, or whether it's maybe worth adding getters on the renderer that we will maintain and map to wherever these live going forward. The benefit there is we can then change the implementations without breaking addons, and also provide backwards compatibility to 1.x pre-1.9.2 and post-1.9.2 without asking add-on developers to make changes. Otherwise, we just need to know that this is something else we'll have to ping developers about when making other updates for 2.0.

@limzykenneth
Copy link
Member

I think the getters on the renderer sounds reasonable although I'm trying to think if there are any potential problems there, especially when it comes to documenting differences between renderers and future additional renderer implementation.

@limzykenneth
Copy link
Member

@davepagurek If we expose states value on a case by case basis depending on current addon compatibility, eg. for WebGL renderer expose a getter to this._renderer.uMVMatrix, while not exposing others that we don't see being used, would that be sufficient? Any future addons that want to access the other states and do it through the states object instead. That way we limit the implementation to individual renderer special cases and also act as a stop gap for addons until they are updated eventually.

@davepagurek
Copy link
Contributor

I think that makes sense, we probably only just need the matrices for WebGL. Then between this and worldToScreen + screenToWorld we probably can close this as complete

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

No branches or pull requests

5 participants