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

Add organizer dependencies #1069

Closed
wants to merge 2 commits into from
Closed

Add organizer dependencies #1069

wants to merge 2 commits into from

Conversation

cjhowedev
Copy link

Adds the indices of the in edges of organizer vertices, so that dependencies can be checked for completion before running a vertex's task. Also simplifies how the is_top_level field is calculated to reuse the passed in dependencies.

@skypjack skypjack self-requested a review September 18, 2023 15:16
@skypjack skypjack self-assigned this Sep 18, 2023
@skypjack skypjack added the triage pending issue, PR or whatever label Sep 18, 2023
@skypjack skypjack changed the base branch from master to wip September 18, 2023 15:16
@skypjack
Copy link
Owner

Just to be clear, with dependencies you mean parent nodes, right?

@cjhowedev
Copy link
Author

Yes, I can rename it if that helps clarify.

@skypjack
Copy link
Owner

Well, I don't like parent either but I'd change the name to <let's find the right one>.
The fact is that dependencies makes me think to something on which the node depends rather than something that depends on the node, if you get what I mean.

@Innokentiy-Alaytsev
Copy link
Contributor

  1. Dependent
  2. Successor
  3. Derivative
  4. Subject (?)

@cjhowedev
Copy link
Author

cjhowedev commented Sep 19, 2023

Hmm, if we don't use the terminology parent, I don't think we should use children either. I think the best path forward is to rename both to successors/predecessors to reduce confusion as to where they are in the graph. Also, to be clear, I think in edges for a vertex should correlate to predecessors, while out edges for a vertex should correlate to successors.

@skypjack
Copy link
Owner

Approved. 👍

This change adds a `predecessors` method to the vertices returned from
the `oragnizer::build` method. The `children` of a vertex are also
renamed to `successors` for consistency. The existing `children` method
still exists for backwards compatibility.
@codecov-commenter
Copy link

codecov-commenter commented Sep 19, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (33b5982) 100.00% compared to head (37fab82) 100.00%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff            @@
##               wip     #1069   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          142       142           
  Lines        25156     25238   +82     
=========================================
+ Hits         25156     25238   +82     
Files Changed Coverage Δ
src/entt/entity/organizer.hpp 100.00% <100.00%> (ø)
test/entt/entity/organizer.cpp 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cjhowedev
Copy link
Author

Updated the commit message to match others. I also decided to keep children around for backwards compatibility, and have created successors/predecessors. The code should be ready for review.

@skypjack skypjack added feature request feature request not yet accepted and removed triage pending issue, PR or whatever labels Sep 21, 2023
@@ -178,11 +178,13 @@ class basic_organizer final {
* @param vtype True if the vertex is a top-level one, false otherwise.
* @param data The data associated with the vertex.
* @param edges The indices of the children in the adjacency list.
* @param edges The indices of the dependencies in the adjacency list.
Copy link
Owner

Choose a reason for hiding this comment

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

edges -> predecessors?

@@ -178,11 +178,13 @@ class basic_organizer final {
* @param vtype True if the vertex is a top-level one, false otherwise.
Copy link
Owner

Choose a reason for hiding this comment

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

vtype should go away, isn't it?


/**
* @brief Returns the list of successors for a given vertex.
* @return The list of successors' indices for the vertex.
*/
const std::vector<std::size_t> &children() const noexcept {
Copy link
Owner

Choose a reason for hiding this comment

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

Keep this one here is fine but I'd add a [[deprecated("use ::successors")]] too.

@@ -63,15 +63,30 @@ TEST(Organizer, EmplaceFreeFunction) {
ASSERT_FALSE(graph[2u].top_level());
ASSERT_FALSE(graph[3u].top_level());

ASSERT_EQ(graph[0u].children().size(), 2u);
Copy link
Owner

Choose a reason for hiding this comment

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

Since children still exists, please, do not drop its tests or the coverage tool will complain. 👍

Copy link
Owner

Choose a reason for hiding this comment

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

Ping @cjhowedev 🙂

skypjack added a commit that referenced this pull request May 6, 2024
@skypjack skypjack added the solved available upstream or in a branch label May 6, 2024
@skypjack
Copy link
Owner

skypjack commented May 6, 2024

Available on wip, updated to the latest version of the library.

@skypjack skypjack closed this May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request feature request not yet accepted solved available upstream or in a branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants