support dedicated executor for component nodes #1781

gezp · 2021-09-23T03:01:46Z

Signed-off-by: zhenpeng ge zhenpeng.ge@qq.com

Related to #1774

it offer an “executor per component” behavior for container, we can use use_dedicated_thread parameter to enable this.

fujitatomoya

probably we would want to discuss on implementation details, and test would be ideal too.

rclcpp_components/src/component_manager.cpp

iuhilnehc-ynos · 2021-09-27T00:09:21Z

I think it's better to add an item use_dedicated_thread in https://github.com/ros2/rcl_interfaces/blob/9a44202bbaf76e19fde21da0e90d8afbe89bd263/composition_interfaces/srv/LoadNode.srv.

fujitatomoya · 2021-09-27T03:59:49Z

@iuhilnehc-ynos i thought that too, i think that also makes sense.

gezp · 2021-09-27T04:14:46Z

@iuhilnehc-ynos is it necessary to add a new item use_dedicated_thread in LoadNode.srv ? what do you think about using extra_arguments in LoadNode.srv to load parameter use_dedicated_thread like use_intra_process_comms (https://github.com/ros2/rclcpp/blob/master/rclcpp_components/src/component_manager.cpp#L155-L163) ?

iuhilnehc-ynos · 2021-09-27T05:04:49Z

@gezp

is it necessary to add a new item use_dedicated_thread in LoadNode.srv ? what do you think about using extra_arguments in LoadNode.srv to load parameter use_dedicated_thread like use_intra_process_comms (https://github.com/ros2/rclcpp/blob/master/rclcpp_components/src/component_manager.cpp#L155-L163) ?

From my point of view, I think that use_dedicated_thread is different from use_intra_process_comms, the former is just used inside component_manager to create a special thread, to set it with a parameter of Node seems a bit strange, the latter use_intra_process_comms is an option of Node, which could be a parameter.

I think that the user does not want to get use_dedicated_thread info by ros2 param list a_node.

What do you think?

iuhilnehc-ynos · 2021-09-27T06:49:32Z

@gezp

Sorry, I made a mistake. After reconsideration, I think it's OK to use extra_arguments.

gezp · 2021-09-28T13:36:43Z

according to suggestions from @fujitatomoya @iuhilnehc-ynos , i load use_dedicated_thread parameter at runtime, in other word, use request->extra_arguments to check if use dedicated thread for loaded node. please review again!

SteveMacenski · 2021-10-13T17:21:33Z

@fujitatomoya @iuhilnehc-ynos Can you take another look? Zhengpeng made the requested changes:

Node loaded per-node
Uses extra_arguments in LoadNode.srv to specify
Removes global parameter

fujitatomoya · 2021-10-14T21:29:36Z

@gezp i think i am good to go with test cases.

SteveMacenski · 2021-10-14T21:43:21Z

With your approval, what's the block to merging? I actually don't know for rclcpp, do you require 2 approvals from maintainers?

fujitatomoya · 2021-10-14T21:52:04Z

i am not sure if that is a rule, but i would like to hear at least one maintainer on this one.

gezp · 2021-10-19T06:55:12Z

i fixed some issues and added test for use_dedicated_thread

fix destructor : only nodes which doesn't use dedicated executor need be removed from exec.
load parameter use_dedicated_thread before unique_id_++: when use_dedicated_thread is not boolean, it throws a exception and unique_id_ shouldn't change.

SteveMacenski · 2021-10-28T16:19:08Z

Any update?

ros-discourse · 2021-11-01T20:52:33Z

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2021-10-28/22947/1

fujitatomoya

lgtm

rclcpp_components/test/test_component_manager_api.cpp

fujitatomoya · 2021-11-06T20:20:24Z

CI:

Linux
Linux-aarch64
macOS
Windows

gezp · 2021-11-07T03:12:41Z

i rebased this PR, please retrigger CI. @fujitatomoya

fujitatomoya · 2021-11-08T05:04:05Z

CI:

Linux
Linux-aarch64
macOS
Windows

iuhilnehc-ynos

@gezp

Some minor comments, could you please check them?

rclcpp_components/include/rclcpp_components/component_manager.hpp

rclcpp_components/src/component_manager.cpp

rclcpp_components/test/test_component_manager_api.cpp

gezp · 2021-11-08T13:13:39Z

thanks for your suggestions @fujitatomoya @iuhilnehc-ynos , i updated this PR.

iuhilnehc-ynos

LGTM

fujitatomoya · 2021-11-15T15:36:52Z

CI(Retry):

Linux
Linux-aarch64
macOS
Windows

SteveMacenski · 2021-11-15T19:16:54Z

CI(Retry):

Love that green ❤️

fujitatomoya · 2021-11-15T19:51:26Z

@ivanpauno @wjwwood @jacobperron requesting maintainer review on this.

ivanpauno

Adding this option seems like too much configurability to me.
I would rather have an specialized component manager in another repo for this use case or just use manual composition when needed.

I understand this can be handy though, so I'm happy with it if another maintainer approves.

I have left some minor comments in the implementation.

rclcpp_components/include/rclcpp_components/component_manager.hpp

ivanpauno · 2021-11-16T20:12:33Z

rclcpp_components/src/component_manager.cpp

        exec->remove_node(wrapper.second.get_node_base_interface());
      }
    }
+    for (auto & exec : dedicated_executors_) {
+      exec.second->cancel();


cancel() has some issues, particularly it's ignored if it happens before the call to spin().
That can causes races if someone starts and interrupt the component manager fast.

I would rather use spin_until_future_complete(), and complete the promise when you need to cancel the executor thread.

rclcpp/rclcpp_components/src/component_manager.cpp

Lines 257 to 258 in 5fcb789

dedicated_executors_[node_id] = exec;

dedicated_threads_[node_id] = std::thread([exec]() {exec->spin();});

It is added to dedicated_executors_ right before being spun, so it should always have been called spin to be on that map in the first place. I suppose the only way to make it more bulletproof is if those 2 lines are swapped so that its only added to dedicated_executors_ after its been spinning.

Swapping the lines doesn't help.

The issue is that though the thread is already spawned, you don't actually know when there will be a context switch and spin() will be actually called.
In an extremely unlucky scenario, the thread is spawn and an unload request is received before spin() is called, resulting in the cancellation request being lost.

the "right way" to do it is:

std::promise<void> prom; dedicated_threads_[node_id] = std::thread([exec, cancel_token=prom->get_future()]() {exec->spin_until_future_complete(cancel_token);}); dedicated_promises_[node_id] = std::move(promise);

- dedicated_executor->second->cancel(); + dedicated_promise->second->set_value();

(the code will need some more changes than that, that's just the idea)

there's no race possible in that case, as a future/promise channel is stateful.

Thanks for the specific suggestion!

thanks for more details about how to implement, it's helpful to me.

rclcpp_components/src/component_manager.cpp

jacobperron

I tend to agree with @ivanpauno; rather than introducing a new option, I think it would be better to create a different component container that uses this dedicated executor approach (e.g. then you don't necessarily need the extra argument use_dedicated_thread).

Technically, we can create a new executable where we pass a different executor to ComponentManager or create an extension of ComponentManager to use multiple single-threaded executors (like in this PR).

For example, we ended up creating a custom container in the domain_bridge that extends ComponentManager to add a new option for setting the domain ID, versus adding more options in the base class here.

jacobperron · 2021-11-17T22:39:36Z

rclcpp_components/src/component_manager.cpp

-      options.use_intra_process_comms(extra_argument.get_value<bool>());
-    }
-  }
-


I'm curious about the decision to move this out of create_node_options. Is there a particular reason?

It technically could break any subclasses that were depending on this option being set in this method (though I admit it is not particularly well-documented).

thanks for pointing this problem, i agree with you that subclasses could be affected. use_intra_process_comms and use_dedicated_thread both are also extra argument, so i wanted to avoid duplicated code by using map to search extra argument, but i didn't notice this problem.

SteveMacenski · 2021-11-29T18:42:41Z

We could create both, but I think that the one using the single threaded executor is the most useful.

Agreed, I figure it better to be 'complete' though and its only an extra 6 lines of code to provide it out of the box initially.

fujitatomoya · 2021-11-29T19:57:40Z

Currently we have a component_container and a component_container_mt executable, so I think that adding a third executable component_container_isolated (name can be bikesheded later) makes sense.

I am also good to go with this.

gezp · 2021-11-30T02:35:51Z

i use a simple way to implement isolate executor container, which only support dedicated single threaded executor.

accoding to @SteveMacenski suggestions, ComponentManagerIsolated derived from ComponentManager maybe a better choice to support for single and multi threaded executors, but there are some duplicated code, i'm not sure whether to use this approach, i would like to hear your opinions.

in addition, i use promise and spin_until_future_complete() instead of executor->cancel(), but it can't work corrently, i don't know why. so i use both of prom.set_value() and executor->cancel() to cancel executor. could you help me to solve this problem? @ivanpauno

SteveMacenski · 2021-11-30T19:11:43Z

ComponentManagerIsolated derived from ComponentManager maybe a better choice to support for single and multi threaded executors, but there are some duplicated code, i'm not sure whether to use this approach, i would like to hear your opinions.

Its hard to tell without seeing it. It sounds like you would only need to change on_load_node and on_unload_node which are both virtual so you can override them. on_unload_node looks like a trivial amount of code (< 15 functional lines) and is all executor-model-specific so I think that's fine to copy paste over and modify for the N executors.

on_load_node seems like a bit more code. The functional part that would need to be changed looks like https://github.com/ros2/rclcpp/blob/master/rclcpp_components/src/component_manager.cpp#L217-L220. You could easily create a new, virtual function called something like add_node_to_executor() or something who's implementation is just these lines. Then in your implementation, you can override it to set it to use the N executors.

I think that sounds the cleanest to me. You'll obviously need a little bit of code in the constructor / destructor as well, but little to any duplication if you call the base class constructor / destructors' to handle their own resources.

What do you think?

gezp · 2021-12-01T03:40:53Z

i updated PR.

create ComponentManagerIsolated derived from ComponentManager , and override on_load_node and on_unload_node.
create a new type of container component_container_isolated by using ComponentManagerIsolated.

need ComponentManagerIsolated be templated by ExecutorT to support both of single and multi threaded executors？then, a new type of container component_container_mt_isolated should be created to support multi threaded executors, right?

ivanpauno · 2021-12-01T17:05:13Z

rclcpp_components/include/rclcpp_components/component_manager_isolated.hpp

+  std::unordered_map<uint64_t, std::shared_ptr<rclcpp::Executor>> dedicated_executors_;
+  std::unordered_map<uint64_t, std::thread> dedicated_threads_;
+  std::unordered_map<uint64_t, std::promise<void>> dedicated_promises_;


Maybe you can create a custom struct for the three values (executor, thread, promise) and create only one unordered map.
You could also use a tuple instead of defining a custom struct (defining a custom struct makes the code more readable than using tuples).

Agreed. A custom struct is the cleanest way

ivanpauno · 2021-12-01T17:12:38Z

rclcpp_components/src/component_manager_isolated.cpp

+}
+
+void
+ComponentManagerIsolated::on_load_node(


Some of the code here is repeated from the base class and could be shared with it.
Same in on_unload_node().

Anyways the approach seems reasonable!

Feel free to ignore this comment

ivanpauno · 2021-12-01T17:19:50Z

rclcpp_components/test/test_component_manager_api.cpp

+
+TEST_F(TestComponentManager, components_api)
+{
+  test_components_api(false);


It's nicer to use a parameterized test for this, see https://github.com/google/googletest/blob/main/docs/advanced.md#value-parameterized-tests.

If you feel that's an overkill here, add scoped traces so it's more clear where the errors come from.
You could also return an AssertionResult to avoid this pitfall.

SteveMacenski · 2021-12-01T18:55:32Z

need ComponentManagerIsolated be templated by ExecutorT to support both of single and multi threaded executors

I would like that, yes. I think that would make this future proof. This may require making the normal ComponentManager to also be templated and remove the constructor argument, which I think is also a good move. You can simply instantiate ExecutorT in the constructor -- that way we can use the same API for everything.

then, a new type of container component_container_mt_isolated should be created to support multi threaded executors, right?

👍

SteveMacenski · 2021-11-30T19:04:47Z

rclcpp_components/src/component_container_isolated.cpp

@@ -0,0 +1,33 @@
+// Copyright 2019 Open Source Robotics Foundation, Inc.


This should be your own copyright 😄

Technically, the copyright can remain OSRF, and there may be (legal) reasons to prefer leaving it as-is for both the contributor and OSRF. But, there's precedent for allowing additional copyright holders in ROS 2 source code. I would defer to @clalancette and/or @tfoote for clarity.

It's certainly not required to put your own name there, but it's also totally fine as long as the changes to the file are non-trivial or it's a new file (which is the case here).

So, putting yourself there is ok, and leaving it as-is is also ok. :)

So, putting yourself there is ok, and leaving it as-is is also ok. :)

Yes, exactly. Since this is a non-trivial amount of new work, it is perfectly acceptable to make yourself the copyright holder. In that case, you would change the line to be // Copyright 2021 <my name here>. On the other hand, it is also perfectly fine to "assign" the copyright to the Open Source Robotics Foundation, which gives the foundation most of the legal power over the code. Totally up to you which way you want to go.

(the usual disclaimer of "I am not a lawyer" and "this is not legal advice" applies here)

SteveMacenski · 2021-12-01T18:52:21Z

rclcpp_components/include/rclcpp_components/component_manager_isolated.hpp

+protected:
+  RCLCPP_COMPONENTS_PUBLIC
+  void
+  on_load_node(


You'll want to add documentation here like you see in the other .hpp files for auto-doc generation

SteveMacenski · 2021-12-01T18:52:41Z

rclcpp_components/include/rclcpp_components/component_manager_isolated.hpp

+  std::unordered_map<uint64_t, std::shared_ptr<rclcpp::Executor>> dedicated_executors_;
+  std::unordered_map<uint64_t, std::thread> dedicated_threads_;
+  std::unordered_map<uint64_t, std::promise<void>> dedicated_promises_;


Agreed. A custom struct is the cleanest way

SteveMacenski · 2021-12-01T18:59:46Z

rclcpp_components/src/component_manager_isolated.cpp

+  const std::shared_ptr<LoadNode::Request> request,
+  std::shared_ptr<LoadNode::Response> response)
+{
+  (void) request_header;


This entire code block L46-L87 and L103-114 are exactly the same as in the other container.

What you could do instead is what I suggested above. Put the things in L88-L102 into a separate, virtual function with the implementation that is overrided. Then you won't even need to override or implement on_load_node. This is really good not to duplicate code + makes it so any change to the main container are automatically applied here.

If you did that too, it would resolve Ivan's comment above on duplication as well, because you wouldn't have on_load_node at all.

gezp · 2021-12-02T06:19:06Z

i updated PR.

ComponentManagerIsolated is templated by ExecutorT to support both of single and multi threaded executors
add a new type of container component_container_mt_isolated
add new virtual functions add_node_to_executor() and remove_node_from_executor() to avoid duplicated code.

please review again @SteveMacenski @ivanpauno .

SteveMacenski · 2021-12-02T06:53:34Z

rclcpp_components/include/rclcpp_components/component_manager_isolated.hpp

+
+#include "rclcpp_components/component_manager.hpp"
+
+


Its up to core maintainers (@ivanpauno ) but I would rather the other containers follow suit with the ExecutorT so that you can remove the constructor field. The template for type + executor constructor argument are redundant. You can have the same effect by removing all constructor arguments from the existing component_manager.hpp and making that also templated, which constructs its internal executor by instantiating ExecutorT().

I'm not strongly opinioned one way or another - my thoughts are only for consistency which we may not need to strive for here.

Also, needs the function documentation inline (see component_manager.hpp, you added there, just add here too)

Yes, we're not taking any advantage of the template parameter here.
We can delete it or be consistent in the base class, I would just remove it as it doesn't seem to be needed.

(with the template parameter we could've not added the new virtual methods, but both approaches are fine)

SteveMacenski · 2021-12-02T06:56:45Z

This looks really good to me. This has my stamp of approval after the inline hpp method docs are added

ivanpauno

LGTM!

I would prefer to see the component_container_mt_isolated removed before merging.
See my other comment.

rclcpp_components/CMakeLists.txt

gezp · 2021-12-07T03:07:38Z

according to suggestions from @ivanpauno , @wjwwood , @SteveMacenski , i remove component_container_mt_isolated, i also provide a CLI options --use_multi_threaded_executor to support isolated multi_threaded_executor.

ivanpauno · 2021-12-07T13:07:34Z

Linux
Linux-aarch64
macOS
Windows

ivanpauno · 2021-12-07T13:52:53Z

@gezp this seems to need a rebase to make CI pass, thanks!

Signed-off-by: zhenpeng ge <zhenpeng.ge@qq.com> update default value of use_dedicated_executor Signed-off-by: zhenpeng ge <zhenpeng.ge@qq.com> create ComponentManagerIsolated Signed-off-by: zhenpeng ge <zhenpeng.ge@qq.com> use executor wrapper Signed-off-by: zhenpeng ge <zhenpeng.ge@qq.com> remove duplicated code Signed-off-by: zhenpeng ge <zhenpeng.ge@qq.com> template ComponentManagerIsolated and add component_container_mt_isolated Signed-off-by: zhenpeng ge <zhenpeng.ge@qq.com> update test Signed-off-by: zhenpeng ge <zhenpeng.ge@qq.com> updaate CMakeLists.txt Signed-off-by: zhenpeng ge <zhenpeng.ge@qq.com> add some docs comment for function Signed-off-by: zhenpeng ge <zhenpeng.ge@qq.com> remove component_container_mt_isolated and add CLI option Signed-off-by: zhenpeng ge <zhenpeng.ge@qq.com>

ivanpauno · 2021-12-07T14:30:32Z

CI:

Linux
Linux-aarch64
macOS
Windows

SteveMacenski · 2021-12-07T16:25:58Z

I approve this, though I would feel better if the normal container also contained the multi-threaded option and removed the separate container, but I leave it to maintainers.

ivanpauno · 2021-12-07T17:01:14Z

I approve this, though I would feel better if the normal container also contained the multi-threaded option and removed the separate container, but I leave it to maintainers.

That seems fine to me.
We could also have only one executable with two options (single/multi threaded, isolated/normal).

We should check the impact of removing the executable, I guess some launch files will break.
If that's the case, we should probably hava a deprecation cycle before removing the existing component_container_mt executable.

I would do that in a new PR instead of here, as deprecating/removing an executable will likely require changes in other core repos, which will block this PR further.

ivanpauno · 2021-12-07T17:25:06Z

Thanks @gezp for the contribution!!

Blast545 · 2021-12-08T14:07:19Z

👨‍🌾 I think this PR broke our Windows CI builds:
See: https://ci.ros2.org/job/ci_windows/15992/
https://ci.ros2.org/view/nightly/job/nightly_win_deb/2198/

The error:

03:30:50   Building Custom Rule C:/ci/ws/src/ros2/rclcpp/rclcpp_components/CMakeLists.txt

03:30:50   test_component_manager.cpp

03:30:50   test_component_manager.vcxproj -> C:\ci\ws\build\rclcpp_components\Debug\test_component_manager.exe

03:30:50   Building Custom Rule C:/ci/ws/src/ros2/rclcpp/rclcpp_components/CMakeLists.txt

03:30:50   test_component_manager_api.cpp

03:30:50 test_component_manager_api.obj : error LNK2001: unresolved external symbol "protected: virtual void __cdecl rclcpp_components::ComponentManagerIsolated<class rclcpp::executors::SingleThreadedExecutor>::add_node_to_executor(unsigned __int64)" (?add_node_to_executor@?$ComponentManagerIsolated@VSingleThreadedExecutor@executors@rclcpp@@@rclcpp_components@@MEAAX_K@Z) [C:\ci\ws\build\rclcpp_components\test_component_manager_api.vcxproj]

03:30:50 test_component_manager_api.obj : error LNK2001: unresolved external symbol "protected: virtual void __cdecl rclcpp_components::ComponentManagerIsolated<class rclcpp::executors::SingleThreadedExecutor>::remove_node_from_executor(unsigned __int64)" (?remove_node_from_executor@?$ComponentManagerIsolated@VSingleThreadedExecutor@executors@rclcpp@@@rclcpp_components@@MEAAX_K@Z) [C:\ci\ws\build\rclcpp_components\test_component_manager_api.vcxproj]

03:30:50 C:\ci\ws\build\rclcpp_components\Debug\test_component_manager_api.exe : fatal error LNK1120: 2 unresolved externals [C:\ci\ws\build\rclcpp_components\test_component_manager_api.vcxproj]

03:30:50 ---
03:30:50 Failed   <<< rclcpp_components [1min 47s, exited with code 1]

This PR was modifying rclcpp_components so CI should have been run up to that package instead of rclcpp.
Can you guys take a look to the problem? @gezp @ivanpauno

This reverts commit 321c74c.

Signed-off-by: zhenpeng ge <zhenpeng.ge@qq.com>

fujitatomoya self-assigned this Sep 23, 2021

fujitatomoya requested changes Sep 24, 2021

View reviewed changes

rclcpp_components/src/component_manager.cpp Outdated Show resolved Hide resolved

rclcpp_components/src/component_manager.cpp Show resolved Hide resolved

gezp requested a review from fujitatomoya October 19, 2021 09:08

fujitatomoya approved these changes Nov 6, 2021

View reviewed changes

rclcpp_components/test/test_component_manager_api.cpp Outdated Show resolved Hide resolved

iuhilnehc-ynos reviewed Nov 8, 2021

View reviewed changes

gezp requested a review from iuhilnehc-ynos November 15, 2021 07:10

iuhilnehc-ynos approved these changes Nov 15, 2021

View reviewed changes

ivanpauno reviewed Nov 16, 2021

View reviewed changes

jacobperron self-requested a review November 17, 2021 23:23

jacobperron reviewed Nov 18, 2021

View reviewed changes

ivanpauno reviewed Dec 1, 2021

View reviewed changes

SteveMacenski requested changes Dec 1, 2021

View reviewed changes

SteveMacenski reviewed Dec 2, 2021

View reviewed changes

ivanpauno approved these changes Dec 3, 2021

View reviewed changes

rclcpp_components/CMakeLists.txt Outdated Show resolved Hide resolved

ivanpauno approved these changes Dec 7, 2021

View reviewed changes

ivanpauno merged commit 321c74c into ros2:master Dec 7, 2021

ivanpauno mentioned this pull request Dec 7, 2021

Add parameter to configure number of thread #1708

Merged

Blast545 added a commit that referenced this pull request Dec 8, 2021

Revert "create component_container_isolated (#1781)"

2b1e2fc

This reverts commit 321c74c.

Blast545 mentioned this pull request Dec 8, 2021

Revert "create component_container_isolated (#1781)" #1840

Closed

gezp mentioned this pull request Dec 9, 2021

fix failed windows ci of PR1781 by removing unnecessary RCLCPP_COMPONENTS_PUBLIC #1843

Merged

Bi0T1N pushed a commit to Bi0T1N/rclcpp that referenced this pull request Dec 15, 2021

create component_container_isolated (ros2#1781)

a4df420

Signed-off-by: zhenpeng ge <zhenpeng.ge@qq.com>

gezp mentioned this pull request Dec 20, 2021

support dynamic composed bringup ros-navigation/navigation2#2750

Merged

7 tasks

alsora mentioned this pull request Feb 1, 2022

Use spin() in component_manager_isolated.hpp #1881

Merged

	dedicated_executors_[node_id] = exec;
	dedicated_threads_[node_id] = std::thread([exec]() {exec->spin();});

		@@ -0,0 +1,33 @@
		// Copyright 2019 Open Source Robotics Foundation, Inc.


		#include "rclcpp_components/component_manager.hpp"

support dedicated executor for component nodes #1781

support dedicated executor for component nodes #1781

Conversation

gezp commented Sep 23, 2021 • edited Loading

fujitatomoya left a comment

Choose a reason for hiding this comment

iuhilnehc-ynos commented Sep 27, 2021

fujitatomoya commented Sep 27, 2021

gezp commented Sep 27, 2021 • edited Loading

iuhilnehc-ynos commented Sep 27, 2021

iuhilnehc-ynos commented Sep 27, 2021

gezp commented Sep 28, 2021

SteveMacenski commented Oct 13, 2021

fujitatomoya commented Oct 14, 2021

SteveMacenski commented Oct 14, 2021 • edited Loading

fujitatomoya commented Oct 14, 2021

gezp commented Oct 19, 2021

SteveMacenski commented Oct 28, 2021

ros-discourse commented Nov 1, 2021

fujitatomoya left a comment

Choose a reason for hiding this comment

fujitatomoya commented Nov 6, 2021 • edited Loading

gezp commented Nov 7, 2021

fujitatomoya commented Nov 8, 2021

iuhilnehc-ynos left a comment

Choose a reason for hiding this comment

gezp commented Nov 8, 2021

iuhilnehc-ynos left a comment

Choose a reason for hiding this comment

fujitatomoya commented Nov 15, 2021

SteveMacenski commented Nov 15, 2021

fujitatomoya commented Nov 15, 2021

ivanpauno left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveMacenski Nov 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobperron left a comment • edited Loading

Choose a reason for hiding this comment

jacobperron Nov 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveMacenski commented Nov 29, 2021

fujitatomoya commented Nov 29, 2021

gezp commented Nov 30, 2021

SteveMacenski commented Nov 30, 2021

gezp commented Dec 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveMacenski commented Dec 1, 2021

Choose a reason for hiding this comment

jacobperron Dec 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gezp commented Dec 2, 2021

SteveMacenski Dec 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveMacenski commented Dec 2, 2021

ivanpauno left a comment

Choose a reason for hiding this comment

gezp commented Dec 7, 2021

ivanpauno commented Dec 7, 2021

ivanpauno commented Dec 7, 2021

ivanpauno commented Dec 7, 2021

SteveMacenski commented Dec 7, 2021

ivanpauno commented Dec 7, 2021

ivanpauno commented Dec 7, 2021

Blast545 commented Dec 8, 2021

gezp commented Sep 23, 2021 •

edited

Loading

gezp commented Sep 27, 2021 •

edited

Loading

SteveMacenski commented Oct 14, 2021 •

edited

Loading

fujitatomoya commented Nov 6, 2021 •

edited

Loading

SteveMacenski Nov 17, 2021 •

edited

Loading

jacobperron left a comment •

edited

Loading

jacobperron Nov 17, 2021 •

edited

Loading

jacobperron Dec 1, 2021 •

edited

Loading

SteveMacenski Dec 2, 2021 •

edited

Loading