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

Improve the heuristic for the camera projection distance #916

Closed
wants to merge 1 commit into from

Conversation

emilk
Copy link
Member

@emilk emilk commented Jan 25, 2023

Base in on the latest known scene size.

Closes #681

I'm not super-happy with how we need to pipe the scene bbox around to accomplish this.

An alternative would be to computer it last, that is: populate the scene with a dummy distance, then compute the final distance after the scene is complete, like we do for point radii. The problem with this is that we also need the projection distance for the transform hieararchy for projecting 2D stuff onto the plane.

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I've added a line to CHANGELOG.md (if this is a big enough change to warrant it)

Base in on the latest known scene size.
@emilk emilk requested a review from Wumpf January 25, 2023 11:11
@@ -107,6 +115,7 @@ impl Viewport {
spaces_info,
space_info,
&ObjectsProperties::default(),
&macaw::BoundingBox::nothing(),
Copy link
Member Author

@emilk emilk Jan 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like how I need to do this either. Why do we need the TransformCache when creating a SpaceView?

default_queried_objects_by_category uses it for objects_with_reachable_transform which only uses the graph part of it, not the actual transforms.

But it is also used to populate the initial cached_transforms, but cached_transforms is already recomputed at the start of each frame, so I don't see the need for it at all (or it could be an Option so we don't need to populate it with a dummy-value on start, or we could populate it with Default).

@Wumpf

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't like it either! The more accurate line of reasoning is afaik that we populate a scene and populating a scene requires transforms.

Relatedly I'd like to either get rid of SpaceInfo altogether or at least make it derived of the transform cache and not the other way round. SpaceInfo as a concept is rather odd both in naming and role - really what it digests for us is "all paths that have a transform and how they relate to all other paths"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixing some aspects of this now while fixing space view generation heuristics

@Wumpf
Copy link
Member

Wumpf commented Jan 25, 2023

The way out of passing around things for heuristics would be to have all elements on ObjectProperties be aware of their state. I had this thing here for that purpose almost checked in:

/// A value that is either determined automatically by some heuristic, or specified by the user.
#[derive(Clone, serde::Deserialize, serde::Serialize)]
#[serde(bound = "T: serde::Serialize, for<'de2> T: serde::Deserialize<'de2>")]
pub enum EditableAutoValue<T>
where
    T: Clone + Default + serde::Serialize,
    for<'de2> T: serde::Deserialize<'de2>,
{
    /// The user explicitely specified what they wanted
    UserEdited(T),

    /// The value is determined automatically.
    ///
    /// We may update this at any time or interpret the value stored here differently under certain circumstances.
    Auto(T),
}

impl<T> Default for EditableAutoValue<T>
where
    T: Clone + Default + serde::Serialize,
    for<'de2> T: serde::Deserialize<'de2>,
{
    fn default() -> Self {
        EditableAutoValue::Auto(T::default())
    }
}

impl<T> EditableAutoValue<T>
where
    T: Clone + Default + serde::Serialize,
    for<'de2> T: serde::Deserialize<'de2>,
{
    pub fn is_auto(&self) -> bool {
        matches!(self, EditableAutoValue::Auto(_))
    }

    /// Gets the value, disregarding if it was user edited or determined by a heuristic.
    pub fn get(&self) -> &T {
        match self {
            EditableAutoValue::Auto(v) | EditableAutoValue::UserEdited(v) => v,
        }
    }
}

We could then update all autovalues whenever we do the propagation

@emilk
Copy link
Member Author

emilk commented Jan 25, 2023

That could work, but takes a bit of care. We would need to update the object properties at the start of each frame, and make sure at least the root objects have properties (or that they all do).

@emilk
Copy link
Member Author

emilk commented Mar 1, 2023

Closed by #1433

@emilk emilk closed this Mar 1, 2023
@emilk emilk deleted the emilk/improve-project-plane-distance-heuristic branch March 1, 2023 14:54
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.

Improve camera frustum length heuristic & editability
2 participants