Return a Result from Camera::viewport_to_world

[tormeh]

Objective

This is a follow-up to 8841

The objective is to create a better UX when something goes wrong in Camera::viewport_to_world and related methods.

Solution

Move from using Option to detailed Result variants. The user no longer has to guess what the error was.

---

Changelog

Improve errors related to functions converting coordinates between camera, viewport, and world.

Migration Guide

Replace code that expects Option with code that expects Result. An easy way is to append a .ok() to the returned Result to transform it into an Option.

Change references to Camera::projection_matrix to Camera::projection_matrix_unchecked.

[alice-i-cecile]

Good stuff: I really like the direction that this is going.

[IceSentry]

It's a bit unfortunate that it makes the code more verbose but I think these changes make sense.

I wonder if the new is_nan checks can affect performance though. Probably not, but I'm still a bit curious.

[IceSentry]

Should this maybe check for is_finite instead of is_nan? I'm not sure if infinite makes sense here.

[tormeh]

This makes me think; Do we want infinite anywhere? Can/Should I use is_finite everywhere instead of is_nan?

[tormeh]

Have done some effort to judge when to do this, but unfortunately I've all but forgotten all the graphics-related knowledge I acquired for the first PR and I'm to lazy to go back to the YouTube videos. Hope I got it right 😅

[tormeh]

It's a bit unfortunate that it makes the code more verbose but I think these changes make sense.

I wonder if the new is_nan checks can affect performance though. Probably not, but I'm still a bit curious.

My limited knowledge says that since the numbers being checked are probably in a fast CPU cache it should be pretty fast, but you never know.

EDIT: Just realized that this doesn't help if the code to check for nulls is not also in the CPU cache... I guess this would vary based on usage.

[tormeh]

@Selene-Amanita Fixed some of the stuff you remarked on in #8841

[tormeh]

I've tried to apply a bit of common sense as to where infinity should be allowed and not (almost nowhere), but I'm honestly fumbling in the dark, so please give that a quick scan

[alice-i-cecile]

Alright, the code quality is up to snuff now :) This is a much nicer and more informative approach to error handling here, but I need to defer to people with more rendering expertise to know if the edge cases are all handled correctly.

[tormeh]

Alright, the code quality is up to snuff now :) This is a much nicer and more informative approach to error handling here, but I need to defer to people with more rendering expertise to know if the edge cases are all handled correctly.

Same lol

[tormeh]

@tormeh Thanks for picking this back up!

Appreciate it. Have never rebased on top of an entire year's worth of changes before 😅

[alice-i-cecile]

Bah, still listing merge conflicts 🤔 Ping me when this is ready please!

[tormeh]

It's getting late here, so the rest will have to be done later. I've gotten to June.

[alice-i-cecile]

Oof :( Feel free to remake the PR if it's easier: rebasing can be quite painful.

[alice-i-cecile]

Adopted in #14931.