blender/blender

Rename camera matrix terminology #127806

New Issue

Open

opened 2024-09-18 15:49:25 +02:00 by Miguel Pozo · 8 comments

Miguel Pozo commented

2024-09-18 15:49:25 +02:00

Member

The Blender terminology for camera matrices doesn't follow the usual naming conventions and can be quite counterintuitive.

We have:

viewmat for the view matrix (this one is fine)
winmat for the projection matrix (window matrix?🤔)
persmat for winmat*viewmat (perspective matrix, which can be easily mistaken as the projection matrix)

I propose to just follow the usual naming conventions, so it would become:

view_mat
projection_mat
view_projection_mat

The Blender terminology for camera matrices doesn't follow the usual naming conventions and can be [quite counterintuitive](https://projects.blender.org/blender/blender/pulls/127718#issuecomment-1295816). We have: - `viewmat` for the view matrix (this one is fine) - `winmat` for the projection matrix ([window matrix](https://www.google.com/search?q=window+matrix)?🤔) - `persmat` for `winmat*viewmat` (perspective matrix, which can be easily mistaken as the projection matrix) I propose to just follow the usual naming conventions, so it would become: - `view_mat` - `projection_mat` - `view_projection_mat`

Miguel Pozo added the

label 2024-09-18 15:49:25 +02:00

Miguel Pozo referenced this issue

2024-09-18 15:50:43 +02:00

Fix #127590: EEVEE: Wrong background motion vectors #127718

Clément Foucault added the

Viewport & EEVEE

labels 2024-09-18 17:05:25 +02:00

Clément Foucault commented

2024-09-18 17:21:32 +02:00

Member

I don't know where this terminology originated from. The only good thing about it is that names are quite short. But that can also be cryptic.

I would like to mention that there are also viewinv, wininv and persinv. I propose:

view_mat_inv or view_mat_inverse
projection_mat_inv or projection_mat_inverse
view_projection_mat_inv or view_projection_mat_inverse

Would like to know the feeling of some core contributors as this touches pretty much all 3D modules. There is over 1000 references of these over the whole codebase. Some of them are inside DNA and some exposed in the RNA through window_matrix, view_matrix, view_perspective. Obviously I don't think renaming RNA is worth it. But it would creates a discrepancy between python and C.

Tagging @JacquesLucke @ideasman42 @Sergey @mont29 @filedescriptor to know their opinions.

I don't know where this terminology originated from. The only good thing about it is that names are quite short. But that can also be cryptic. I would like to mention that there are also `viewinv`, `wininv` and `persinv`. I propose: - `view_mat_inv` or `view_mat_inverse` - `projection_mat_inv` or `projection_mat_inverse` - `view_projection_mat_inv` or `view_projection_mat_inverse` Would like to know the feeling of some core contributors as this touches pretty much all 3D modules. There is over 1000 references of these over the whole codebase. Some of them are inside DNA and some exposed in the RNA through `window_matrix`, `view_matrix`, `view_perspective`. Obviously I don't think renaming RNA is worth it. But it would creates a discrepancy between python and C. Tagging @JacquesLucke @ideasman42 @Sergey @mont29 @filedescriptor to know their opinions.

Sean Kim commented

2024-09-18 21:01:04 +02:00

Member

Just mentioning one other way of specifying an inverse matrix that I've seen (mainly in sculpt / paint code) is imat. I don't have particularly strong thoughts on how we name things here, but I wanted to bring it up in case people really liked it as a third option.

Just mentioning one other way of specifying an inverse matrix that I've seen (mainly in sculpt / paint code) is `imat`. I don't have particularly strong thoughts on how we name things here, but I wanted to bring it up in case people really liked it as a third option.

Clément Foucault commented

2024-09-19 00:38:40 +02:00

Member

It was mentioned in the chat that projection could be shortened to proj.

Another option is to use the same convention as for objects matrices (i.e. object_to_world).
I believe Cycles uses raster for the post projection space but I am not sure it maps 1:1 to projection matrices. For now I propose to use homogeneous as it is what we decided to use for the draw module inside the shaders. But we could also just use projection here too.

So we would have:

world_to_view
view_to_homogeneous
world_to_homogeneous

EDIT: However, we also have RE_GetCameraWindow. And renaming it to RE_get_camera_view_to_homogeneous might not be to everyone's liking.

It was mentioned in the chat that `projection` could be shortened to `proj`. Another option is to use the same convention as for objects matrices (i.e. `object_to_world`). I believe Cycles uses `raster` for the post projection space but I am not sure it maps 1:1 to projection matrices. For now I propose to use `homogeneous` as it is what we decided to use for the draw module inside the shaders. But we could also just use `projection` here too. So we would have: - `world_to_view` - `view_to_homogeneous` - `world_to_homogeneous` EDIT: However, we also have `RE_GetCameraWindow`. And renaming it to `RE_get_camera_view_to_homogeneous` might not be to everyone's liking.

Jacques Lucke commented

2024-09-19 14:44:30 +02:00

Member

I like the x_to_y naming generally for matrices. It makes it obvious what they do and how to compose them, especially in more complex situations. But I'm also totally fine with using industry standard terms for these matrices here. What these are, you'd know better than me.

I like the `x_to_y` naming generally for matrices. It makes it obvious what they do and how to compose them, especially in more complex situations. But I'm also totally fine with using industry standard terms for these matrices here. What these are, you'd know better than me.

Sergey Sharybin commented

2024-09-19 16:17:52 +02:00

Owner

For the inverse, I think we commonly follow _inv suffix. The imat is kinda historical, and should be renamed to world_to_object (that is what imat used to be, and for other cases using i prefix is kinda easily missable).

Surely x_to_y is helping in general case, I am not sure it really helps here. There are well-established terms in the computer graphics for these matrices.
The issue with Cycles's is that it is not that obvious what raster or screen actually is. And there is also ndc. Every time i use them i need to check how exactly they are calculated.
And homogenous sounds a bit vague.

But also any of the proposals here are better than the current state :)

For the inverse, I think we commonly follow `_inv` suffix. The `imat` is kinda historical, and should be renamed to `world_to_object` (that is what `imat` used to be, and for other cases using `i` prefix is kinda easily missable). Surely `x_to_y` is helping in general case, I am not sure it really helps here. There are well-established terms in the computer graphics for these matrices. The issue with Cycles's is that it is not that obvious what `raster` or `screen` actually is. And there is also `ndc`. Every time i use them i need to check how exactly they are calculated. And `homogenous` sounds a bit vague. But also any of the proposals here are better than the current state :)

Miguel Pozo commented

2024-09-19 16:38:41 +02:00

Author

Member

It looks like projmat is already used in some places.

BTW, BLI_math_geom.h functions are a perfect example of the issues with this terminology.
We have "perspective" used for both perspective projection matrices and for view+projection matrices. And "projection" and "window" is used interchangeably.

void projmat_dimensions(const float winmat[4][4],
                        float *r_left,
                        float *r_right,
                        float *r_bottom,
                        float *r_top,
                        float *r_near,
                        float *r_far)
{
  const bool is_persp = winmat[3][3] == 0.0f;

About raster, I have not checked it, but I would assume these are for pixel coordinates (the equivalent of FragCoord).

I like the x_to_y for functions (IMO the draw_view_lib API is as good as it gets), but for the matrices themselves it can get confusing.
RE_get_camera_view_to_homogeneous is kind of bad, but something like view_to_homogeneous_dimensions is even worse and would need more thoughtful renaming.

Also, the concept of a projection matrix probably sounds much more familiar to non-graphics devs than the concept of homogeneous or NDC coordinates.

BTW, when using that terminology for the matrices themselves, I'd say x_from_y is probably a better fit for Blender. So we have:
homogeneous = homogeneous_from_view * view_from_world * world_from_local * vert_position
instead of:
homogeneous = view_to_homogeneous * world_to_view * local_to_world * vert_position

In any case, I personally would go for the least invasive changes that solve the most glaring issues.
There might be better style options, but if that's going to require a massive API redesign instead of a simple batch rename I'd say it's not worth it.

It looks like `projmat` is already used in some places. BTW, `BLI_math_geom.h` functions are a perfect example of the issues with this terminology. We have "perspective" used for both perspective projection matrices and for view+projection matrices. And "projection" and "window" is used interchangeably. ```cpp void projmat_dimensions(const float winmat[4][4], float *r_left, float *r_right, float *r_bottom, float *r_top, float *r_near, float *r_far) { const bool is_persp = winmat[3][3] == 0.0f; ``` About `raster`, I have not checked it, but I would assume these are for pixel coordinates (the equivalent of `FragCoord`). I like the `x_to_y` for functions (IMO the `draw_view_lib` API is as good as it gets), but for the matrices themselves it can get confusing. `RE_get_camera_view_to_homogeneous` is kind of bad, but something like `view_to_homogeneous_dimensions` is even worse and would need more thoughtful renaming. Also, the concept of a projection matrix probably sounds much more familiar to non-graphics devs than the concept of homogeneous or NDC coordinates. BTW, when using that terminology for the matrices themselves, I'd say `x_from_y` is probably a better fit for Blender. So we have: `homogeneous = homogeneous_from_view * view_from_world * world_from_local * vert_position` instead of: `homogeneous = view_to_homogeneous * world_to_view * local_to_world * vert_position` --- In any case, I personally would go for the least invasive changes that solve the most glaring issues. There might be better style options, but if that's going to require a massive API redesign instead of a simple batch rename I'd say it's not worth it.

Bastien Montagne commented

2024-09-19 18:14:58 +02:00

Owner

Regarding renaming RNA, currently we lack a proper way to handle this, so better to avoid it indeed, and live with the difference between C++ code and RNA/Python names. See alos discussion in !126796

But also any of the proposals here are better than the current state :)

Agreed!

Regarding renaming RNA, currently we lack a proper way to handle this, so better to avoid it indeed, and live with the difference between C++ code and RNA/Python names. See alos discussion in !126796 > But also any of the proposals here are better than the current state :) Agreed!

Clément Foucault commented

2024-09-19 18:16:13 +02:00

Member

In any case, I personally would go for the least invasive changes that solve the most glaring issues.
There might be better style options, but if that's going to require a massive API redesign instead of a simple batch rename I'd say it's not worth it.

The issue is that this touches a lot of areas as I pointed out. And having yet another convention that is not enforced or applied over the whole codebase will increase friction.

I would also avoid doing batch rename on the whole codebase in one commit. It is easy to change something else by mistake. Cleaning one API / module at a time seems better to ensure the best semantic for each.

> In any case, I personally would go for the least invasive changes that solve the most glaring issues. There might be better style options, but if that's going to require a massive API redesign instead of a simple batch rename I'd say it's not worth it. The issue is that this touches a lot of areas as I pointed out. And having yet another convention that is not enforced or applied over the whole codebase will increase friction. I would also avoid doing batch rename on the whole codebase in one commit. It is easy to change something else by mistake. Cleaning one API / module at a time seems better to ensure the best semantic for each.

Bastien Montagne referenced this issue

2024-09-19 18:58:34 +02:00

RNA API: Support Proper Deprecation Process #127876

Sign in to join this conversation.

No Label

Animation & Rigging

Automated Testing

Blender Asset Bundle

Dependency Graph

Development Management

Images & Movies

Motion Tracking

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Render Pipeline

Sculpt, Paint & Texture

Video Sequencer

Viewport & EEVEE

Virtual Reality

Asset Browser Project

Blender 2.8 Project

Milestone 1: Basic, Local Asset Browser

Good First Issue

Animation & Rigging

Development Management

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Sculpt, Paint & Texture

Viewport & EEVEE

Needs Info from Developers

Needs Information from User

No Milestone

No project

No Assignees

6 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#127806

No description provided.