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 scaling when using withTarget() and isolatedWithTarget() #22

Closed
wants to merge 1 commit into from
Closed

Fix scaling when using withTarget() and isolatedWithTarget() #22

wants to merge 1 commit into from

Conversation

Syngeo
Copy link
Contributor

@Syngeo Syngeo commented Sep 30, 2018

If the render target's size was different to the application, the scaling
would be incorrect because the projection matrix wasn't adjusted for the
render target's size. This should fix that and restore the original
matrix when done.

If the render target's size was different to the application, the scaling
would be incorrect because the projection matrix wasn't adjusted for the
render target's size. This should fix that and restore the original
matrix when done.
@edwinRNDR
Copy link
Member

Hi @MinteZ, this is cool and I understand exactly why you wrote this patch. But what about the various scenarios in which the projection is not an orthogonal projection or the projection should be kept? I currently rely (not much, but still) on withTarget/isolatedWithTarget's behaviour to not not touch the projection matrix.

Maybe a resetProjection: Boolean=true argument can be added to make the reset behaviour default but optional. I'd love to hear your thoughts on this.

@Syngeo
Copy link
Contributor Author

Syngeo commented Sep 30, 2018

That's a good point, I hadn't thought of that. I like the resetProjection idea and will be happy to implement it once there's a way to find out which matrix to apply.

As for the scaling issue, should the user want to change it, I might have to experiment and see if I can find an alternative solution. I know Processing doesn't seem to have this problem, so it's definitely possible to do this. As of right now, the only fix I can think of would be to keep track of the projection parameters given and whether or not it's orthographic or perspective so that they can be reapplied when drawing to another target. As for how these should be stored, I'm not entirely sure, but I can't think of another way to 'resize' a projection matrix other than storing the parameters and reusing them when creating a new one (unless there's some matrix maths out there for that which would likely make this easier). I would be happy to hear what you think of this or if you have some alternative ideas.

@Syngeo
Copy link
Contributor Author

Syngeo commented Oct 1, 2018

I just had a thought: what if we allowed each render target to hold a projection matrix of its own? It would be easier to maintain their projection that way and they can have their own matrix independent of the Drawer's one (these should either add to or override the Drawer's one). This may require functions in either RenderTarget or Drawer (preferably the former) for changing the RenderTarget's projection, but if it's possible, I think it could be worth it.

@edwinRNDR
Copy link
Member

In essence I don't think you can scale or otherwise recalculate projection matrices as the user's intention is missing from the equation.

I see merit in having an optional projection matrix associated per render target and I am curious how and when this projection matrix would be applied. I imagine non-null projection matrices to override the drawer projection at withTarget and isolatedWithTarget. Then will the render target projection by default be set to ortho(0, width, height ,0, -1, 1) or will it be null?

@Syngeo
Copy link
Contributor Author

Syngeo commented Oct 2, 2018

I think the Drawer's projection matrix should be the default, since it has one already, and people can then override it at will by setting the render target's projection instead. I'm not sure, though, if it should override or be applied on top of the Drawer's matrix (or if the user should be given an option, either as a parameter or a field in Drawer/DrawStyle).

@edwinRNDR
Copy link
Member

I am still in doubt about the amount mechanisms and potential surprises added versus the problem we are trying to solve. Keep in mind that the user can fix it by resetting the projection matrix using a one-liner (drawer.ortho(rt)). I am inclined to leave withTarget as is and focus on documentation efforts in which it is clearly explained that withTarget leaves the projection matrix untouched and it should be reset to match the target's dimensions if that's desired.

@Syngeo
Copy link
Contributor Author

Syngeo commented Oct 7, 2018

Yeah, that would be a lot simpler. In that case, then I'll close this for now. Thanks, though.

I also just noticed I misread your comment when talking about the default projection matrix, so I just want to clarify what I meant: the render target's projection matrix should be null by default, in which case the Drawer's projection matrix would be used for rendering as you said.

@Syngeo Syngeo closed this Oct 7, 2018
@Syngeo Syngeo deleted the bugfix-rendertarget-projection branch October 7, 2018 15:01
@edwinRNDR
Copy link
Member

edwinRNDR commented Oct 7, 2018

Thanks! I definitely appreciate your effort and the conversation, and I am looking forward to your future contributions.

@Syngeo
Copy link
Contributor Author

Syngeo commented Oct 7, 2018

No problem, and thank you too! I'm happy to help.

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

2 participants