composition/proposal: Permit nested canonical links #36
Conversation
EricCousineau-TRI
force-pushed
the
feature-composition-nested-canonical
branch
from
cef1da9 to
de9745d
Compare
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @azeey, @EricCousineau-TRI, @iche033, and @scpeters)
composition/proposal.md, line 194 at r1 (raw file):
canonical link. To keep in line with the rule that interface elements should be frames (since
I'm actually not sure how I feel about this part.
Perhaps we just say "meh, forget abstraction" and still keep this constrained to links.
I think that this is in-line with what we expose for the interface API (#3).
Will change.
EricCousineau-TRI
changed the title
EricCousineau-TRI
force-pushed
the
feature-composition-nested-canonical
branch
from
de9745d to
3e08d0d
Compare
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @azeey, @iche033, and @scpeters)
composition/proposal.md, line 194 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
I'm actually not sure how I feel about this part.
Perhaps we just say "meh, forget abstraction" and still keep this constrained to links.
I think that this is in-line with what we expose for the interface API (#3).Will change.
Done.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @azeey, @EricCousineau-TRI, and @iche033)
a discussion (no related file):
this modifies the composition proposal (SDFormat 1.8), but if we're going to incorporate this in the libsdformat9 pull request, should we target these changes at the SDFormat 1.7 proposal?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @azeey, @iche033, and @scpeters)
a discussion (no related file):
Previously, scpeters (Steve Peters) wrote…
this modifies the composition proposal (SDFormat 1.8), but if we're going to incorporate this in the libsdformat9 pull request, should we target these changes at the SDFormat 1.7 proposal?
Yup! Will try retargeting.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @azeey, @EricCousineau-TRI, and @iche033)
a discussion (no related file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Yup! Will try retargeting.
Er, actually, this is a bit awkward.
Your code PR itself targets SDFormat 1.7 / libsdformat9.3.0(ish), but it arises from the composition proposal that itself is marked as targeting SDFormat 1.8 / libsdformat11.
I'm just going to try and delineate these sections in the proposal itself. If you'd want it split out in separate proposals, can I leave that up to you?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @azeey, @EricCousineau-TRI, and @iche033)
a discussion (no related file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Er, actually, this is a bit awkward.
Your code PR itself targets SDFormat 1.7 / libsdformat9.3.0(ish), but it arises from the composition proposal that itself is marked as targeting SDFormat 1.8 / libsdformat11.
I'm just going to try and delineate these sections in the proposal itself. If you'd want it split out in separate proposals, can I leave that up to you?
Ah, #34 has what I need... Will think about that...
There was a problem hiding this comment.
Retargeted - perhaps this should merge before #34, just so you have the amendment setup?
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @azeey, @iche033, and @scpeters)
a discussion (no related file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Ah, #34 has what I need... Will think about that...
Done.
There was a problem hiding this comment.
Reviewed 3 of 3 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @EricCousineau-TRI, @iche033, and @scpeters)
composition/legacy_behavior.md, line 8 at r3 (raw file):
**Note**: This describes older legacy behavior. Newer features, intended for SDFormat 1.8 / `libsdformat` 11, are described in the [Composition Proposal](/?tut=composition_proposal).
The markdown for the "Composition Proposal" link is not getting converted properly for some reason
http://sdformat.org/tutorials?tut=pose_frame_semantics_proposal&cat=pose_semantics_docs&branch=feature-composition-nested-canonical&repo=https%3A%2F%2Fgithub.com%2FEricCousineau-TRI%2Fsdf_tutorials-1#8-directly-nested-models
Also, I think the link should be /tutorials?tut=composition_proposal
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @EricCousineau-TRI and @iche033)
a discussion (no related file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Done.
thanks for targeting the 1.7 proposal. This proposed change seems fine to me, but I think my changes in #34 conflict with this
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @EricCousineau-TRI, @iche033, and @scpeters)
a discussion (no related file):
Previously, scpeters (Steve Peters) wrote…
thanks for targeting the 1.7 proposal. This proposed change seems fine to me, but I think my changes in #34 conflict with this
I think we should merge them. I'm fine with one of us just doing merge conflict resolution.
Thoughts?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @EricCousineau-TRI, @iche033, and @scpeters)
a discussion (no related file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
I think we should merge them. I'm fine with one of us just doing merge conflict resolution.
Thoughts?
Per comment in #34, I want this to merge first.
EricCousineau-TRI
force-pushed
the
feature-composition-nested-canonical
branch
from
0d33b75 to
8e70dd5
Compare
EricCousineau-TRI
force-pushed
the
feature-composition-nested-canonical
branch
from
8e70dd5 to
87af7e6
Compare
There was a problem hiding this comment.
Reviewable status: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @azeey, @iche033, and @scpeters)
composition/legacy_behavior.md, line 8 at r3 (raw file):
Done.
Now renders as this:
There was a problem hiding this comment.
Reviewed 2 of 2 files at r4.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @EricCousineau-TRI, @iche033, and @scpeters)
pose_frame_semantics/proposal.md, line 1464 at r4 (raw file):
For new phases, the ***Title:*** is italicized. **Amendment**: This section has been updated as part of Amendment 1.
If I'm understanding correctly, this statement will be true once #34 gets merged. But #34 is an amendment to SDFormat 1.7 while at the beginning of this document, Amendment 1 states that it is an amendment toward SDFormat 1.8. Can you clarify?
EricCousineau-TRI
force-pushed
the
feature-composition-nested-canonical
branch
from
6b8725f to
18c5134
Compare
There was a problem hiding this comment.
Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @azeey, @iche033, and @scpeters)
pose_frame_semantics/proposal.md, line 1464 at r4 (raw file):
Previously, azeey (Addisu Taddese) wrote…
If I'm understanding correctly, this statement will be true once #34 gets merged. But #34 is an amendment to SDFormat 1.7 while at the beginning of this document, Amendment 1 states that it is an amendment toward SDFormat 1.8. Can you clarify?
You're right, it's part of Amendment 2.
The chronology of targeting SDFormat 1.8 and 1.7 tripped me up 😅
I also got confused thinking that #30 made it's way here.
Will mention that Amendment 1's parsing stages are effectively in the composition proposal, which is confusing AF...
There was a problem hiding this comment.
Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @azeey, @iche033, and @scpeters)
pose_frame_semantics/proposal.md, line 1464 at r4 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
You're right, it's part of Amendment 2.
The chronology of targeting SDFormat 1.8 and 1.7 tripped me up 😅
I also got confused thinking that #30 made it's way here.Will mention that Amendment 1's parsing stages are effectively in the composition proposal, which is confusing AF...
Done. Called this out, and swapped amendment ID's to reflect versions, not chronology of when we made the changes.
There was a problem hiding this comment.
Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @azeey, @EricCousineau-TRI, and @iche033)
a discussion (no related file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Per comment in #34, I want this to merge first.
I said it looks fine to me, but I'm not sure about targeting these changes to the canonical link to SDFormat 1.7. It seems like a bigger change than supporting nested models, which have already been supported to some extent since SDFormat 1.5. I need to think about that aspect of this PR a bit more.
In the meantime, I could graft your formatting of this PR into #34
There was a problem hiding this comment.
Reviewed 1 of 1 files at r5.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @EricCousineau-TRI and @iche033)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @iche033 and @scpeters)
a discussion (no related file):
Previously, scpeters (Steve Peters) wrote…
I said it looks fine to me, but I'm not sure about targeting these changes to the canonical link to SDFormat 1.7. It seems like a bigger change than supporting nested models, which have already been supported to some extent since SDFormat 1.5. I need to think about that aspect of this PR a bit more.
In the meantime, I could graft your formatting of this PR into #34
Gotcha. So when you said "conflict", you didn't mean merge conflict, but a content conflict?
Yes, please use the amendment formatting so at least the deltas are a bit more anchored.
Regarding this change, then, this should target SDFormat 1.8?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @EricCousineau-TRI and @iche033)
a discussion (no related file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Gotcha. So when you said "conflict", you didn't mean merge conflict, but a content conflict?
Yes, please use the amendment formatting so at least the deltas are a bit more anchored.
Regarding this change, then, this should target SDFormat 1.8?
No, I meant merge conflict, I think I just hadn't processed that it was going to 1.7. I think it's possible to get to 1.7, but I need to think about it more. I'll update #34 so it can be merged first
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @iche033 and @scpeters)
a discussion (no related file):
Previously, scpeters (Steve Peters) wrote…
No, I meant merge conflict, I think I just hadn't processed that it was going to 1.7. I think it's possible to get to 1.7, but I need to think about it more. I'll update #34 so it can be merged first
SGTM. For now, I say we lean towards the conservative side and retarget this PR to SDFormat 1.8.
However, I will hold off until #34 merges first with the grafted changes, then replay these commits on that.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @iche033 and @scpeters)
a discussion (no related file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
SGTM. For now, I say we lean towards the conservative side and retarget this PR to SDFormat 1.8.
However, I will hold off until #34 merges first with the grafted changes, then replay these commits on that.
Done. Concluded waffling per VC: This will target SDFormat 1.7, and will merge before #34.
|
FYI @iche033 Your review is now optional - but please feel free to comment if you see something awry! |
There was a problem hiding this comment.
Reviewed 2 of 2 files at r4.
Reviewable status:complete! all files reviewed, all discussions resolved
Per comment here:
gazebosim/sdformat#316 (review)
This change is