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 foreach not remembering nodes between iterations #1304

Closed
wants to merge 58 commits into from

Conversation

loopspace
Copy link
Contributor

Motivation for this change

Fixes #1303 by remembering the last node between iterations of a foreach loop on a path.

Checklist

Please signoff your commits to explicitly state your agreement to the Developer Certificate of Origin. If that is not possible you may check the boxes below instead:

@loopspace
Copy link
Contributor Author

(Still a bit new to contributing to a Big Project ...)

The latest commit adds what I think is a more flexible approach to saving and restoring these macros and dimensions, which I've then extended to include \tikztostart (for to paths) and \tikz@tangent. There may be other macros and dimensions that need to be added but with this code that will be much easier to do.

My naming convention might not quite fit with the PGF ethos ... happy to change if so.

@@ -2580,28 +2580,64 @@

\def\tikz@fchar oreach{\tikz@foreach}%

\def\tikz@foreach{%
\def\tikzforeach@smugglers@cove{}
\let\tikztostart=\relax
Copy link
Member

Choose a reason for hiding this comment

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

Maybe move this to tex/generic/pgf/frontendlayer/tikz/libraries/tikzlibrarytopaths.code.tex where \tikztostart (and \tikztotarget) originally come from.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had a look at tikzlibrarytopaths.code.tex and while that is where \tikztostart is used, it is never defined there. The definitions are contained in the core TikZ parser in that whenever TikZ encounters a coordinate then it sets \tikztostart to something sensible so that if the next command is a to path then the start is set up correctly. Similarly with \tikz@tangent (which is mainly used in the calc library).

So I'm happy to move it, but just want to double-check first.

}


\def\tikz@patched@foreach{%
Copy link
Member

Choose a reason for hiding this comment

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

I guess you renamed this for testing purposes, but it needs to be renamed back to \tikz@foreach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops!

Comment on lines +2586 to +2628
\def\tikzforeach@smuggle@macro#1{%
\pgfutil@ifx#1\relax{}{%
\expandafter
\pgfutil@g@addto@macro
\expandafter
\tikzforeach@smugglers@cove
\expandafter
{%
\expandafter\def\expandafter#1\expandafter{#1}%
}%
}%
}

\def\tikzforeach@smuggle@dimen#1{%
\pgfutil@ifx#1\relax{}{%
\expandafter
\pgfutil@g@addto@macro
\expandafter
\tikzforeach@smugglers@cove
\expandafter
{%
\expandafter#1\expandafter=\the#1\relax%
}%
}%
}

\def\tikzforeach@smugglers@loot{%
%
\tikzforeach@smuggle@macro\tikz@moveto@waiting%
\tikzforeach@smuggle@macro\tikztostart%
\tikzforeach@smuggle@macro\tikz@tangent%
%
\tikzforeach@smuggle@dimen\tikz@lastx%
\tikzforeach@smuggle@dimen\tikz@lasty%
\tikzforeach@smuggle@dimen\tikz@lastxsaved%
\tikzforeach@smuggle@dimen\tikz@lastysaved%
%
}
Copy link
Member

Choose a reason for hiding this comment

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

I like this approach (and the naming scheme 😁), but I think it would be even better if we could make this more general using pgfkeys, so that a user can register their own loot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you happy with just remember macro and remember dimension for the keys? I've not found any other keys in the tikz namespace with those names.

@hmenke
Copy link
Member

hmenke commented Mar 27, 2024

Fixes #356
Fixes #1047
Fixes #1303
Fixes #1313

@loopspace
Copy link
Contributor Author

That sounds do-able, I would be inclined to keep the current methods and simply wrap them in keys, then the lower-level methods are available to library authors.

I'll also implement the other things you pointed out.

muzimuzhi and others added 24 commits April 2, 2024 19:06
to avoid a subtle constraint of `/.add` that
    /.add={<arg1>}{<arg2>}  % and
    /.add={<arg1>} {<arg2>}
are different and the second is, in most cases, wrong usage.

Signed-off-by: muzimuzhi <[email protected]>
1in=72.27pt, not 72.72pt.
* docs: fix typos

Signed-off-by: muzimuzhi <[email protected]>
Caused by luaotfload v3.24, the first "bad" commit is
    latex3/luaotfload@ca49685

Signed-off-by: muzimuzhi <[email protected]>
dependabot bot and others added 20 commits April 2, 2024 19:06
Bumps [actions/github-script](https://github.com/actions/github-script) from 6 to 7.
- [Release notes](https://github.com/actions/github-script/releases)
- [Commits](actions/github-script@v6...v7)

---
updated-dependencies:
- dependency-name: actions/github-script
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [actions/upload-artifact](https://github.com/actions/upload-artifact) from 3 to 4.
- [Release notes](https://github.com/actions/upload-artifact/releases)
- [Commits](actions/upload-artifact@v3...v4)

---
updated-dependencies:
- dependency-name: actions/upload-artifact
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
`maxprintline` defaults to 9999 since l3build 2023-03-22.
https://github.com/latex3/l3build/blob/main/CHANGELOG.md#2023-03-22

Signed-off-by: Yukai Chou <[email protected]>
- Replace deprecated `ifluatex` package with `iftex`
- Drop setting input encoding to `utf8`
  `utf8` has been the new default since LaTeX2e release 2018-04-01.
- Drop setting `T1` font encoding for `dvisvgm`
  `dvisvgm` has supported OpenType fonts for years.

Signed-off-by: Yukai Chou <[email protected]>
and upload a second artifact containing pdf and all aux files

Signed-off-by: Yukai Chou <[email protected]>
In the code:

```
\draw (A) foreach \nd in {B,C} { -- (\nd) };
```
then the line to `C` is drawn from `A`, not `B`.  This is because the
foreach loop does not update the macro that remembers that the last
coordinate was a node.  This fixes that issue.

See pgf-tikz#1303

Signed-off-by: Andrew Stacey <[email protected]>
Signed-off-by: Andrew Stacey <[email protected]>
This adds in the saving code for `\tikztostart` and `\tikz@tangent`,
plus provides a more robust and maintainable method of saving the
various macros and dimensions by creating a single (global) token list
that will restore them to their given values upon invocation.

Signed-off-by: Andrew Stacey <[email protected]>
The functionality of customized l3build target (`target_list` entry)
`revisionfile` has been replaced with `l3build tag` in
9efb7e4 (refactor(ci)!: use l3build tagfiles, 2021-12-15),
and the definition of `revisionfile()` has been removed in the same commit.

Signed-off-by: Yukai Chou <[email protected]>
Fix in context -- "question" should be singular:
<quote>
    I wish to start with the question “What is TikZ?”
</quote>

Signed-off-by: Alan D. Salewski <[email protected]>
…1310)

Fix in context (in section "Repeating Things: The Foreach Statement"):
<quote>
    Normally, when a list item '...' is encountered, there should already have
    been /two/ list items before it, which were numbers.
</quote>

Signed-off-by: Alan D. Salewski <[email protected]>
The keys are `remember macro` and `remember dimension`.

Also `\tikz@foreach` was erroneously called `\tikz@patched@foreach`
(copied from my original testing file).

Signed-off-by: Andrew Stacey <[email protected]>
@loopspace
Copy link
Contributor Author

In trying to retrospectively sign my commits, I think I messed up the version history. I'll create a fresh branch of the current pgf repo and re-implement my patch, then resubmit it.

@muzimuzhi
Copy link
Member

I'll create a fresh branch of the current pgf repo and re-implement my patch, then resubmit it.

Or you can force push to fixforeach branch.

@loopspace
Copy link
Contributor Author

I'll create a fresh branch of the current pgf repo and re-implement my patch, then resubmit it.

Or you can force push to fixforeach branch.

I did that, but then it's listing a whole slew of commits from the original as if they were fresh commits on this branch. Maybe that's not a problem and that when it's merged into the main branch then those will all get recognised as being the same commits as the original ones. I just don't know!

Well, let me say that I'm quite happy to create a fresh branch and add in my changes, signing them properly this time - if it's needed.

But I guess the most important thing first is to make sure the code is right and worry about the version structure afterwards.

@muzimuzhi
Copy link
Member

I did that, but then it's listing a whole slew of commits from the original as if they were fresh commits on this branch.

Err GitHub thinks all commits merged by loopspace@f2fa2e4 were authored by original authors but committed by you. Not sure if that's normal behavior of a merge commit.

@loopspace
Copy link
Contributor Author

I've made a new branch with the code changes where I've signed-off my commit from the outset. I'll close this pull request and open a new one from that branch, hopefully that won't have all the weird merges.

@loopspace
Copy link
Contributor Author

New pull request: #1316

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.

\foreach behavior inside a \draw