three.js icon indicating copy to clipboard operation
three.js copied to clipboard

Camera: Added fovMode

Open mrdoob opened this issue 4 years ago • 25 comments

Similarly to the Camera's Sensor Fit in Blender, this lets the user configure the in which dimension the fov is being applied: vertical, horizontal or auto. I'm also proposing to set it to auto by default.

This affects mobiles specially. Previously the examples looked like this:

Screen Shot 2020-06-10 at 2 38 00 PM

With the new default it looks like this:

Screen Shot 2020-06-10 at 2 37 48 PM

Which I think is what you would expect when the render looks like this in landscape:

Screen Shot 2020-06-10 at 2 43 08 PM

Not sure about the name fovMode. Any suggestions?

mrdoob avatar Jun 10 '20 21:06 mrdoob

/fyi @elalish

mrdoob avatar Jun 10 '20 21:06 mrdoob

Yeah, that seems like a good one to me. I might introduce that configuration to model-viewer as well.

elalish avatar Jun 10 '20 22:06 elalish

@WestLangley Any chance you can help with the correct hFov and vFov formulas?

mrdoob avatar Jun 10 '20 23:06 mrdoob

I like this change 👍

The name might be a big vague, though. When I saw the title of the PR I though this was going to be a function to help adjust the view of the camera to fit an object in the view.

Maybe something like setFovFit or setFovAxis?

gkjohnson avatar Jun 11 '20 00:06 gkjohnson

Renamed it to fovMode but fovFit may also work? fovAxis also sounds good to me.

mrdoob avatar Jun 11 '20 00:06 mrdoob

Can you explain what camera.fov represents now? It used to be the vertical field-of-view in degrees. Is it still the vertical-field-of-view?

Also, how would you describe what getEffectiveFOV() returns?

WestLangley avatar Jun 11 '20 01:06 WestLangley

Can you explain what camera.fov represents now? It used to be the vertical field-of-view in degrees. Is it still the vertical-field-of-view?

camera.fov is the field-of-view in degrees but, when fovMode is auto it's vertical or horizontal depending on the aspect ratio.

Also, how would you describe what getEffectiveFOV() returns?

Not sure what that method is for 😅

mrdoob avatar Jun 11 '20 01:06 mrdoob

Not sure what that method is for

It is for when camera.zoom !== 1. TBH, I was unaware we were no longer using it.

WestLangley avatar Jun 11 '20 01:06 WestLangley

I personally favor this approach compared to #21190. To me, it's okay if the final semantics of fov depends on another property. According to this PR, it's the field-of-view in degrees. Whether or not it is the vertical or horizontal depends on fovMode. That seems understandable.

Sidenote: Couldn't we remove getEffectiveFOV()? The method is neither used in the examples nor in the editor. If users still require this value, they can compute it on app level.

Mugen87 avatar Mar 16 '21 10:03 Mugen87

I do not think fov should have multiple meanings, so I am not in favor of this.

The API we have works fine.

I support the solution I proposed in https://github.com/mrdoob/three.js/pull/21190#issuecomment-774738902. Alternatively,hfov could be implemented with a getter/setter, but that is an implementation detail.

WestLangley avatar Mar 17 '21 01:03 WestLangley

@Mugen87 said:

Couldn't we remove getEffectiveFOV()? The method is neither used in the examples nor in the editor.

I suggest we avoid using "it's not used in the examples" as a justification to remove a feature. There are multiple methods that are not used in the examples.

The examples exist to demonstrate three.js features. Add an example, instead of removing the feature.

WestLangley avatar Mar 17 '21 02:03 WestLangley

I suggest we avoid using "it's not used in the examples" as a justification to remove a feature. There are multiple methods that are not used in the examples.

Fair enough! I should have also mentioned that I've never seen the method used in the wild for the last couple of years.

Mugen87 avatar Mar 17 '21 09:03 Mugen87

Fair enough! I should have also mentioned that I've never seen the method used in the wild for the last couple of years.

Granted.

It may have been used in CanvasRenderer, or by users currrently to calculate the visible height at a given distance.

Thing is, I expect most users do not have the math skills to derive the formula used in the method. That may be a reason to justify retaining it.

WestLangley avatar Mar 17 '21 13:03 WestLangley

To compute projection matrix with .fov as horizontal instead of vertical

this.perspectiveCamera.aspect = w / h;
this.perspectiveCamera.updateProjectionMatrix();   
if(this.fovAxis==='horizontal'){
    let e = this.perspectiveCamera.projectionMatrix.elements;
    e[5]=this.perspectiveCamera.aspect;
    e[0]=1;
}

Sorry if this is off-topic but I just implemented this for a friend, and then found this thread, so I thought I'd drop it in here.

manthrax avatar Nov 29 '21 13:11 manthrax

For the record, I still prefer what I said previously.

WestLangley avatar Dec 01 '21 14:12 WestLangley

What is fovMode ?deeply explanation, please.

Ben6jamin avatar Dec 06 '22 11:12 Ben6jamin

@Ben6jamin Please ask for help/explanations at the forum.

Mugen87 avatar Dec 06 '22 11:12 Mugen87

@Ben6jamin You can also ask to https://chat.openai.com/chat but the answer may be kind of made up 😁

Screen Shot 2022-12-07 at 11 51 43 AM

mrdoob avatar Dec 07 '22 02:12 mrdoob

@WestLangley Seems like you were the only person opposing this change. Any chance you have changed your mind?

mrdoob avatar Dec 07 '22 02:12 mrdoob

I prefer https://github.com/mrdoob/three.js/pull/21190#issuecomment-774738902 instead of FOVmode.

Actually, horizontalFov could be implemented as a getter/setter. (That is how we handle SpotLight.power.) I like that approach.

WestLangley avatar Dec 07 '22 03:12 WestLangley

What I'm proposing here is to follow Blender's Sensor Fit feature:

Screen Shot 2022-12-07 at 1 52 18 PM

It would be set to auto by default, which may break things but doing camera.fovMode = 'vertical' should fix it.

The benefit is that models will fit in any viewport by default without the developer having to know what horizontal/vertical fov is. They will not need to understand these concepts unless they have to.

What you're suggesting is forcing new users to understand these concepts from the get go. Any projects that new users do will result in models not fitting the viewport in mobile phones until they understand horizontal/vertical fov concepts. And chances are they never will...

This change would result in better looking content by default and I can't imagine any side effects, but I would like to hear any side effects you are seeing and why you're blocking it 🤔

mrdoob avatar Dec 07 '22 05:12 mrdoob

I am not blocking it. I don't like blockers. I am just expressing my preference.

Why can't camera.fov mean just one thing?

I think the answer is, it can. I think you can implement your auto-fit feature without redefining the meaning of camera.fov.

With the getter/setter I proposed, users could do camera.hFov = value if they want. Or, as you prefer, the fov could be set for them automatically by your sensor-fit feature.

I expect it is the feature that you really care about, not the parameterization. At least I hope so. :-)

WestLangley avatar Dec 07 '22 06:12 WestLangley

@mrdoob It is late here. Perhaps I am misunderstanding what you mean by fovMode...

WestLangley avatar Dec 07 '22 06:12 WestLangley

I am not blocking it. I don't like blockers. I am just expressing my preference.

It's probably a language thing but if you say you prefer a different approach to me it sounds that you're not okay with what's being proposed and the different approach should be used instead.

Then the issue gets put on hold until I have time to understand your reasoning and try to reach an agreement.

What happened here is that many other issues piled up on top and I forgot about it.

I expect it is the feature that you really care about, not the parameterization. At least I hope so. :-)

I care about the quality of the content being produced and the user experience.

If I go to a website that uses three.js on my mobile and the model doesn't fit in the viewport (many of the examples have this issue) my first reaction is trying to think if there's anything the library could do so the developer doesn't need to know/worry about this in order to provide a good experience.

@mrdoob It is late here. Perhaps I am misunderstanding what you mean by fovMode...

Currently we have this:

Screen Shot 2022-12-07 at 3 57 58 PM

The idea is to change the current behavior so instead of a "vertical field of view" it uses the largest dimension of the viewport by default.

The user could then control the behavior by setting fovMode to auto (default), horizontal or vertical.

mrdoob avatar Dec 07 '22 07:12 mrdoob