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 Point display to respect formatting parameters #1116

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

owenbrooks
Copy link

This changes Point Display formatting to match formatting of other geometry types, including Isometry, Scale, and Rotation.
Fixes issue #1115 since it passes through the desired precision formatting parameter.

Example:

let point = nalgebra::Point3::new(1.12345678, 2.12345678, 3.12345678);
println!("{point:.4}");

Before:

{1.12345678, 2.12345678, 3.12345678}

After:

Point {
  ┌        ┐
  │ 1.1235 │
  │ 2.1235 │
  │ 3.1235 │
  └        ┘

}

This changes Point formatting to match formatting of other geometry types, including Isometry, Scale, and Rotation.
Fixes issue dimforge#1115 since it passes through the desired precision formatting parameter.
@Ralith
Copy link
Collaborator

Ralith commented Jun 6, 2022

I think it would be better to just forward to the underlying Vector's fmt. Reduces verbosity and handles flags other than precision correctly as well.

@owenbrooks
Copy link
Author

That does seem simpler, thanks. I've made that change. Which other flags were you expecting it to handle when forwarding? From what I can see it looks like the underlying matrix formatting ignores most parameters aside from precision. I'm wondering if it would be worth forwarding fmt for the other geometry types too.

@Ralith
Copy link
Collaborator

Ralith commented Jun 7, 2022

Ah, if Matrix::display isn't handling e.g. padding and signs properly then we at least have a single place to fix that. What other types currently having Display impls did you have in mind?

write!(f, ", {}", *comp)?;
}

write!(f, "Point {{")?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd rather omit this wrapping for concision, personally. It's also unconventional for Display impls in general.

@owenbrooks
Copy link
Author

The Isometry, Scale, Rotation, and Translation types all have similar Display impls that only pass through precision and have similar wrapping with “Typename {“ and “}”. I would prefer omitting the wrapper myself too but would probably want it to be consistent.

@Ralith
Copy link
Collaborator

Ralith commented Jun 7, 2022

Ah, good call.

@RReverser
Copy link

(reporter of the referenced issue here) Hm, I actually liked the concise single-line form of Point, while the new form makes it a bit more verbose.

I guess it does make it more consistent with other formats though...

@owenbrooks
Copy link
Author

(reporter of the referenced issue here) Hm, I actually liked the concise single-line form of Point, while the new form makes it a bit more verbose.

I guess it does make it more consistent with other formats though...

Ah, have you tried debug printing? It's concise and actually does follow all format specifiers:

let point = nalgebra::Point3::new(1.12345678, 2.12345678, 3.12345678);
println!("{point:.4?}");

Gives [1.1235, 2.1235, 3.1235]

@RReverser
Copy link

Ah, have you tried debug printing? It's concise and actually does follow all format specifiers:

Huh, I haven't! It's actually a bit surprising that Debug formatting in this case looks more like what I'd expect from Display (being more human-readable and all) while Display formatting in this PR is more like what I'd expect from Debug (showing type names and all).

@Ralith
Copy link
Collaborator

Ralith commented Jun 7, 2022

Conversely, fancy many-line 2D layout with unicode block drawing is worlds apart from what you'd expect in Debug.

@RReverser
Copy link

Conversely, fancy many-line 2D layout with unicode block drawing is worlds apart from what you'd expect in Debug.

Sounds like we're in agreement. For matrix types I'd expect neat block drawing in Display, but type names and such in Debug.

In case of Point it's just a bit different because, personally, I wouldn't want either of Display or Debug to show a vertical block but rather show values in line for convenience like Point does today.

@owenbrooks
Copy link
Author

I've tried to reduce these inconsistencies in #1119. Would appreciate feedback!

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

3 participants