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

diamond shape: keep aspect when checking against minimum ht/wd #1042

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

Conversation

muzimuzhi
Copy link
Member

Motivation for this change

Currently, when a diamond shape is enlarged when checking against minimum height/width, the shape aspect is not kept. This PR fixes the problem.

Example

\documentclass{article}
\usepackage{tikz}
\usetikzlibrary{shapes.geometric}

\begin{document}
% shape aspect is initially 1 
\begin{tikzpicture}[nodes={diamond, draw}]
  \node[minimum width=50pt] {x};
  \node[minimum height=50pt] at (2,0) {x};
\end{tikzpicture}
\end{document}

image

Checklist

Please check the boxes below and signoff your commits to explicitly
state your agreement to the Developer Certificate of Origin:

@muzimuzhi
Copy link
Member Author

Hmm, pgfmanual reads "the aspect (set by /pgf/aspect/) is a recommendation" for shapes diamond and cylinder.

\begin{key}{/pgf/aspect=\meta{value} (initially 1.0)}
The aspect is a recommendation for the quotient of the width and the
height of a shape. This key calls the macro |\pgfsetshapeaspect|.
\end{key}

Though the relation between /pgf/shape aspect and /pgf/aspect is not explicitly documented, the above doc suggests what this PR tries to fix is a documented "feature". I still think that minimum height/width has higher precedence than shape aspect is strange.

Therefore this PR may end in one of two ways:

  1. Take it as a documented feature and maybe document the relation between /pgf/shape aspect and /pgf/aspect.
  2. Change the precedence for both diamond and cylinder, which is certainly a breaking change.

@hmenke
Copy link
Member

hmenke commented Aug 20, 2021

Unfortunately, we can't do that. What we can do, however, is change the default value for shape aspect to unset and use it if the user explicitly sets a value.

TODO: adjust for shapes cylinder and cloud
Signed-off-by: muzimuzhi <[email protected]>
@muzimuzhi
Copy link
Member Author

muzimuzhi commented Aug 20, 2021

Please review if the current implementation is "what we can do". If so, I will do the same for shapes cylinder and cloud.

Also can the aspect staff common in libs shapes.geometric and shapes.symbols be moved to a separate file, probably named sth ends with shapes.common.code.tex?

Feels like we can only add features if there's a crack in the manual wall. :)


Examples, new

\documentclass{article}
\usepackage{tikz}
\usetikzlibrary{shapes.geometric}

\begin{document}
\tikzset{nodes={diamond, draw}, baseline=0pt}

\def\demo#1{%
  \def\tmp{#1}%
  \ifx\tmp\empty default\else \texttt{#1}\fi:
  \begin{tikzpicture}[style/.expanded={#1}]
    \node[minimum width=50pt] {x};
    \node[minimum height=50pt] at (2,0) {x};
  \end{tikzpicture}
}

\demo{}\par
\demo{shape aspect=none}

\foreach \v in {1, 2, .5} {
  \foreach \k in {shape aspect, aspect} {
    \demo{\k=\v}\qquad
  }\par
}
\end{document}

image

option sets the ratio between width and height of the shape. Unlike
|/pgf/aspect| which only recommends the ratio, this key ensures it.

If the special value |none| is given, the ratio assurance is canceled.
Copy link
Member

Choose a reason for hiding this comment

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

Why the special none value? I would have just left it empty.

Comment on lines +216 to +221
\ifx\pgf@lib@temp\pgf@nonetext
\pgfkeys{/pgf/aspect=1}%
\else
\pgfkeys{/pgf/aspect=#1}%
\fi
\pgfkeyssetvalue{/pgf/shape aspect}{#1}%
Copy link
Member

Choose a reason for hiding this comment

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

If the default was empty the code could be simplified (untested).

Suggested change
\ifx\pgf@lib@temp\pgf@nonetext
\pgfkeys{/pgf/aspect=1}%
\else
\pgfkeys{/pgf/aspect=#1}%
\fi
\pgfkeyssetvalue{/pgf/shape aspect}{#1}%
\pgfqkeys{/pgf}{aspect/.expanded=\pgfutil@ifxempty\pgf@lib@temp{1}{#1}}%
\pgfkeyssetvalue{/pgf/shape aspect}{#1}%

Same for the other conditionals.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants