Fixed issue with creating instances of type InterpMotion<float> #476

shumi12321 · 2020-06-03T08:46:12Z

In InterpMotion class, method integrate() is not properly declared and defined which does not allow instancing of that class with type other than double.
This is proposed fix for this issue.

resolves #475

This change is

shumi12321 · 2020-06-03T08:47:34Z

Link to issue: #475

shumi12321 · 2020-06-03T09:16:48Z

Also added same fix for ScrewMotion class.

…pe float

shumi12321 · 2020-06-03T10:43:25Z

Added support for float type continuous collision objects.

SeanCurtis-TRI

+@SeanCurtis-TRI for feature review

Reviewable status: 0 of 7 files reviewed, all discussions resolved (waiting on @SeanCurtis-TRI)

SeanCurtis-TRI

Thanks for this contribution. :) Please see my comments below.

Reviewed 2 of 2 files at r1, 2 of 2 files at r2, 1 of 1 files at r3, 8 of 8 files at r4.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @shumi12321)

include/fcl/math/motion/interp_motion.h, line 79 at r4 (raw file):

  /// @brief Integrate the motion from 0 to dt
  /// We compute the current transformation from zero point instead of from last integrate time, for precision.
  bool integrate(S dt) const;

nit: This change is to enable something very specific: InterpMotion<float>::integrate(). No matter what else we do, we should introduce a unit test that proves that it works.

include/fcl/math/motion/interp_motion-inl.h, line 48 at r4 (raw file):

//==============================================================================
extern template
class FCL_EXPORT InterpMotion<float>;

nit: This is not a forward "declaration". This is informing the compiler that InterpMotion<float> has already been built and will be available to the linker from some arbitrary compilation object. However, this is not true. In fact, attempting to use an InterpMotion<float> will fail to link. This is not apparent because there are zero unit tests on this class. :(

So, we have a number of things we can/should do:

Definitely remove this extern template declaration (and in all the other files where you've added it for float). This has to be done no matter what else we do.
This next is optional but highly preferred. We should add some unit tests for this class. That would make FCL distinctly better. (For example, I created the beginnings of that unit test and it revealed one bug already.) If you'd like the work I've done, then I can arrange to get you those changes (plenty of options there).

include/fcl/math/motion/screw_motion-inl.h, line 48 at r4 (raw file):

//==============================================================================
extern template
class FCL_EXPORT ScrewMotion<float>;

nit: Just marking the instances of extern template that need to be removed.

include/fcl/narrowphase/collision-inl.h, line 54 at r4 (raw file):

FCL_EXPORT
std::size_t collide(
    const CollisionObject<float>* o1,

nit: Just marking the instances of extern template that need to be removed.

include/fcl/narrowphase/collision-inl.h, line 71 at r4 (raw file):

FCL_EXPORT
std::size_t collide(
    const CollisionGeometry<float>* o1,

nit: Just marking the instances of extern template that need to be removed.

include/fcl/narrowphase/collision_object-inl.h, line 48 at r4 (raw file):

//==============================================================================
extern template
class FCL_EXPORT CollisionObject<float>;

nit: Just marking the instances of extern template that need to be removed.

include/fcl/narrowphase/continuous_collision-inl.h, line 59 at r4 (raw file):

//==============================================================================
extern template
float continuousCollide(

nit: Just marking the instances of extern template that need to be removed.

(All of the changes to this file.)

src/math/motion/interp_motion.cpp, line 44 at r4 (raw file):

template
class InterpMotion<float>;

nit: Just marking the instances of extern template that need to be removed.

src/math/motion/screw_motion.cpp, line 44 at r4 (raw file):

template
class ScrewMotion<float>;

nit: Just marking the instances of extern template that need to be removed.

src/narrowphase/collision.cpp, line 46 at r4 (raw file):

template
std::size_t collide(
    const CollisionObject<float>* o1,

nit: Just marking the instances of extern template that need to be removed. (all of the changes of this file.)

src/narrowphase/collision_object.cpp, line 44 at r4 (raw file):

template
class CollisionObject<float>;

nit: Just marking the instances of extern template that need to be removed.

src/narrowphase/continuous_collision.cpp, line 45 at r4 (raw file):

//==============================================================================
template
float continuousCollide(

nit: Just marking the instances of extern template that need to be removed.

All of the changes of this file.

src/narrowphase/continuous_collision_object.cpp, line 44 at r4 (raw file):

template
class ContinuousCollisionObject<float>;

nit: Just marking the instances of extern template that need to be removed.

shumi12321 · 2020-06-03T15:00:22Z

include/fcl/math/motion/interp_motion-inl.h, line 48 at r4 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: This is not a forward "declaration". This is informing the compiler that InterpMotion<float> has already been built and will be available to the linker from some arbitrary compilation object. However, this is not true. In fact, attempting to use an InterpMotion<float> will fail to link. This is not apparent because there are zero unit tests on this class. :(

So, we have a number of things we can/should do:

Definitely remove this extern template declaration (and in all the other files where you've added it for float). This has to be done no matter what else we do.

This next is optional but highly preferred. We should add some unit tests for this class. That would make FCL distinctly better. (For example, I created the beginnings of that unit test and it revealed one bug already.) If you'd like the work I've done, then I can arrange to get you those changes (plenty of options there).

I added definitions in .cpp files for those forward declarations. Is that ok or do you still want me to remove extern templates?

SeanCurtis-TRI

Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @shumi12321)

include/fcl/math/motion/interp_motion-inl.h, line 48 at r4 (raw file):

Previously, shumi12321 wrote…

I added definitions in .cpp files for those forward declarations. Is that ok or do you still want me to remove extern templates?

I believe the preference is against adding them. Here's the underlying belief and I'm open to being persuaded otherwise.

We assume that double is the most common scalar (so, we just include it in the library directly).
We assume that other scalars (e.g., float) are used sufficiently infrequently that we don't want to increase the library size and compile time for everyone.

So, the implication is that float users will be compiling their own instantiations from the template definition directly.

Thoughts?

SeanCurtis-TRI

Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @shumi12321)

a discussion (no related file):
Regarding testing: I've got a commit that I can contribute to your PR. It might save you some effort in setting up the boilerplate. Let me know if you want it and if you'd prefer receiving a patch, me pushing to your branch, whatever.

shumi12321 · 2020-06-03T15:57:17Z

a discussion (no related file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Regarding testing: I've got a commit that I can contribute to your PR. It might save you some effort in setting up the boilerplate. Let me know if you want it and if you'd prefer receiving a patch, me pushing to your branch, whatever.

That would be great :)

SeanCurtis-TRI

Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @shumi12321)

a discussion (no related file):

Previously, shumi12321 wrote…

That would be great :)

Do you have a preference for how that contribution arises? I can:

post a gist with the patch.
submit a PR to your fork against this branch
push directly to the branch (given permissions)

shumi12321 · 2020-06-04T10:10:05Z

a discussion (no related file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Do you have a preference for how that contribution arises? I can:

post a gist with the patch.

submit a PR to your fork against this branch

push directly to the branch (given permissions)

I added you as collaborator on my forked repo so you can push it there to master branch.

shumi12321 · 2020-06-04T10:10:59Z

include/fcl/math/motion/interp_motion-inl.h, line 48 at r4 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I believe the preference is against adding them. Here's the underlying belief and I'm open to being persuaded otherwise.

We assume that double is the most common scalar (so, we just include it in the library directly).

We assume that other scalars (e.g., float) are used sufficiently infrequently that we don't want to increase the library size and compile time for everyone.

So, the implication is that float users will be compiling their own instantiations from the template definition directly.

Thoughts?

Ok, I will remove forward declaration for float types to speed up compiling for users who will not use floats.

shumi12321 · 2020-06-04T11:44:26Z

include/fcl/math/motion/screw_motion-inl.h, line 48 at r4 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: Just marking the instances of extern template that need to be removed.

Removed

shumi12321 · 2020-06-04T11:44:37Z

include/fcl/narrowphase/collision-inl.h, line 54 at r4 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: Just marking the instances of extern template that need to be removed.

Removed

shumi12321 · 2020-06-04T11:44:50Z

include/fcl/narrowphase/collision-inl.h, line 71 at r4 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: Just marking the instances of extern template that need to be removed.

Removed

shumi12321 · 2020-06-04T11:44:58Z

include/fcl/narrowphase/collision_object-inl.h, line 48 at r4 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: Just marking the instances of extern template that need to be removed.

Removed

shumi12321 · 2020-06-04T11:45:11Z

include/fcl/narrowphase/continuous_collision-inl.h, line 59 at r4 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: Just marking the instances of extern template that need to be removed.

(All of the changes to this file.)

Removed

shumi12321 · 2020-06-04T11:45:31Z

src/math/motion/interp_motion.cpp, line 44 at r4 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: Just marking the instances of extern template that need to be removed.

Not an extern template here.

shumi12321 · 2020-06-04T11:45:35Z

src/math/motion/screw_motion.cpp, line 44 at r4 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: Just marking the instances of extern template that need to be removed.

Not an extern template here.

shumi12321 · 2020-06-04T11:45:40Z

src/narrowphase/collision.cpp, line 46 at r4 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: Just marking the instances of extern template that need to be removed. (all of the changes of this file.)

Not an extern template here.

shumi12321 · 2020-06-04T11:45:45Z

src/narrowphase/collision_object.cpp, line 44 at r4 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: Just marking the instances of extern template that need to be removed.

Not an extern template here.

shumi12321 · 2020-06-04T11:45:50Z

src/narrowphase/continuous_collision.cpp, line 45 at r4 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: Just marking the instances of extern template that need to be removed.

All of the changes of this file.

Not an extern template here.

shumi12321 · 2020-06-04T11:45:57Z

src/narrowphase/continuous_collision_object.cpp, line 44 at r4 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: Just marking the instances of extern template that need to be removed.

Not an extern template here.

SeanCurtis-TRI

Reviewed 4 of 5 files at r5, 3 of 6 files at r6.
Reviewable status: 12 of 15 files reviewed, 12 unresolved discussions (waiting on @SeanCurtis-TRI and @shumi12321)

a discussion (no related file):

Previously, shumi12321 wrote…

I added you as collaborator on my forked repo so you can push it there to master branch.

I've pushed a new commit:

It adds a skeleton unit test (specifically, I started testing constructors as a reality check).
- Ideally, the constructor test would be finished for the five constructors.
- Per this PR we definitely need to add a test showing that we can integrate InterpMotion<S> for S = double, float.
I also fixed some issues with the constructors that became apparent when I wrote the test -- it couldn't pass because the default constructor wasn't really initialized to identities. :-/

a discussion (no related file):
I noticed you're new to github and I'm immensely pleased that we're your first contribution. :) As it appears you're new to this, I'd like to give you some lightweight guidance that'll make your life a bit easier.

You've made your modifications directly in your fork's master branch. Your life will be much simplified if you always do modifications in dedicated branches and PR from there. Right now, you're going to have some merging headaches in your local branch when we finally merge this PR to github's master.
- The primary reason for this headache is we're going to squash all the commits down to a single commit. However, your master branch has a bunch of commits (which have the same end effect), but the fact that it's different commits will cause merge headaches. When we get to merging I can give you some guidance on how to update your master without too much headache.
- Github has some nice documentation discussing this work flow: https://guides.github.com/introduction/flow/
We use reviewable to do code reviews -- it's got some of the nicest workflows for doing so. It comes along with some techniques and conventions that are not immediately clear.
- Reviewable uses keywords like "nit", "minor", "btw", "fyi", "done". Starting a comment with one of these comments will have a special effect.
- You'll notice that a lot of my comments start with "nit". That keyword indicates that I've flagged something minor with a simple resolution. I don't expect that it needs discussion. As such, if you resolve it, you can hit the "Done" button on the comment (or type "done") and the comment will automatically be marked as resolved -- no further work for me. :) The keyword "minor" has the same meaning as "nit".
- If something is marked with "nit" or "minor" it is supposed to communicate that the reviewer is calling out a defect in your code. You may disagree it's a defect -- it happens all the time. At that point, responding with your reasoning is appropriate. Usually, one party persuades the other party and the resolution evolves organically.
- The keywords "BTW" or "FYI" are intended as passing thoughts -- they should never be used to indicate a change you should make. As such, you are completely at liberty to simply disregard them (or, if it's a suggestion that you like, you are equally free to make a change). It's 100% up to you.

include/fcl/narrowphase/continuous_collision-inl.h, line 97 at r6 (raw file):

    ContinuousCollisionResult<double>& result);

nit: looks like you still have a unneeded modification here. Let's go ahead and leave this file completely untouched.

src/math/motion/interp_motion.cpp, line 44 at r4 (raw file):

Previously, shumi12321 wrote…

Not an extern template here.

While it's not an extern template, it should still be removed because this will cause the system to build the float version, but it will lead to linker errors.

Essentially, you should think of the extern template declarations in header files and these explicit instantiations in .cpp files as working pairs (the standard C++ "declaration/definition" duo). One without the other is usually bad. So, this is only justified by the extern template declaration we no longer have, and if we were to include the declaration, it would require this to be meaningful. So, we add or remove both of them as whole pairs.

src/math/motion/screw_motion.cpp, line 44 at r4 (raw file):

Previously, shumi12321 wrote…

Not an extern template here.

See note on interp_motion.cpp on why this needs to be removed; essentially, none of these .cpp files should have any modifications.

src/narrowphase/collision.cpp, line 46 at r4 (raw file):

See note on interp_motion.cpp on why this needs to be removed; essentially, none of these .cpp files should have any modifications.
See note on interp_motion.cpp on why this needs to be removed; essentially, none of these .cpp files should have any modifications.

src/narrowphase/collision_object.cpp, line 44 at r4 (raw file):

Previously, shumi12321 wrote…

Not an extern template here.

See note on interp_motion.cpp on why this needs to be removed; essentially, none of these .cpp files should have any modifications.

src/narrowphase/continuous_collision.cpp, line 45 at r4 (raw file):

Previously, shumi12321 wrote…

Not an extern template here.

See note on interp_motion.cpp on why this needs to be removed; essentially, none of these .cpp files should have any modifications.

src/narrowphase/continuous_collision_object.cpp, line 44 at r4 (raw file):

Previously, shumi12321 wrote…

Not an extern template here.

See note on interp_motion.cpp on why this needs to be removed; essentially, none of these .cpp files should have any modifications.

test/math/motion/test_interp_motion.cpp, line 75 at r6 (raw file):

  {
    // Construct from start (R_FS, p_FSo) and goal (R_FG, p_FGo).

Let's fill these in.

test/math/motion/test_interp_motion.cpp, line 91 at r6 (raw file):

}
}

And this is where you'll add a simple TYPED_TEST instantiating an InterpMotion<S> (as with the previous test), confirming that interpolation is behaving as expected.

InterpMotion has no unit tests at all. This floats on top of pr_476 and adds the skeleton of a unit test. While writing the unit test, I encountered issues: 1. The default constructor (documented to be identity) wasn't. 2. The constructors were incredibly erratic. Some did partial initialization. This commit adds addresses issues as encountered. # Conflicts: # include/fcl/math/motion/interp_motion-inl.h

SeanCurtis-TRI

Reviewed 3 of 6 files at r6, 1 of 1 files at r7.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @shumi12321)

sherm1 · 2020-06-04T16:11:47Z

A random drive-by comment for @shumi12321:
People are usually disappointed when they switch from double to float hoping to improve performance. It is very common for code to run at the same speed or even slower in float. You give up a lot in accuracy with float computation, so you should definitely measure the results and not assume better performance. It is true that float will save memory over double for big meshes and sometimes that can also provide a performance improvement due to better cache locality. But an overall performance improvement is by no means guaranteed.

shumi12321 · 2020-06-14T13:31:43Z

a discussion (no related file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I've pushed a new commit:

It adds a skeleton unit test (specifically, I started testing constructors as a reality check).

Ideally, the constructor test would be finished for the five constructors.

Per this PR we definitely need to add a test showing that we can integrate InterpMotion<S> for S = double, float.

I also fixed some issues with the constructors that became apparent when I wrote the test -- it couldn't pass because the default constructor wasn't really initialized to identities. :-/

Nice

shumi12321 · 2020-06-14T13:33:43Z

a discussion (no related file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I noticed you're new to github and I'm immensely pleased that we're your first contribution. :) As it appears you're new to this, I'd like to give you some lightweight guidance that'll make your life a bit easier.

You've made your modifications directly in your fork's master branch. Your life will be much simplified if you always do modifications in dedicated branches and PR from there. Right now, you're going to have some merging headaches in your local branch when we finally merge this PR to github's master.

The primary reason for this headache is we're going to squash all the commits down to a single commit. However, your master branch has a bunch of commits (which have the same end effect), but the fact that it's different commits will cause merge headaches. When we get to merging I can give you some guidance on how to update your master without too much headache.

Github has some nice documentation discussing this work flow: https://guides.github.com/introduction/flow/

We use reviewable to do code reviews -- it's got some of the nicest workflows for doing so. It comes along with some techniques and conventions that are not immediately clear.

Reviewable uses keywords like "nit", "minor", "btw", "fyi", "done". Starting a comment with one of these comments will have a special effect.

You'll notice that a lot of my comments start with "nit". That keyword indicates that I've flagged something minor with a simple resolution. I don't expect that it needs discussion. As such, if you resolve it, you can hit the "Done" button on the comment (or type "done") and the comment will automatically be marked as resolved -- no further work for me. :) The keyword "minor" has the same meaning as "nit".

If something is marked with "nit" or "minor" it is supposed to communicate that the reviewer is calling out a defect in your code. You may disagree it's a defect -- it happens all the time. At that point, responding with your reasoning is appropriate. Usually, one party persuades the other party and the resolution evolves organically.

The keywords "BTW" or "FYI" are intended as passing thoughts -- they should never be used to indicate a change you should make. As such, you are completely at liberty to simply disregard them (or, if it's a suggestion that you like, you are equally free to make a change). It's 100% up to you.

The whole purpose of this fork is to submit this PR. I will remove it as soon as PR is done.
Thanks for this info, didn't know that

shumi12321 · 2020-06-14T13:34:20Z

include/fcl/math/motion/interp_motion.h, line 79 at r4 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: This change is to enable something very specific: InterpMotion<float>::integrate(). No matter what else we do, we should introduce a unit test that proves that it works.

Unit tests were added as another commit...

SeanCurtis-TRI

Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @shumi12321)

include/fcl/math/motion/interp_motion.h, line 79 at r4 (raw file):

Previously, shumi12321 wrote…

Unit tests were added as another commit...

I'll emphasize the fact that I put in the skeleton of the unit tests, but haven't finished them off to the degree that they should be for this PR. I assumed you'd do that.

Fixed issue with creating instances of type InterpMotion<float>

9124137

shumi12321 mentioned this pull request Jun 3, 2020

Incorrect declaration of virtual function makes project not compilable with float type #475

Open

Fixed issue with creating instances of type ScrewMotion<float>

3023add

Fixed issue with creating continuous collision objects of template ty…

857d82f

…pe float

josipsumeckispread added 2 commits June 3, 2020 13:23

Fixed issue with creating collision objects of template type float

e90d404

Added support for float CollisionObject

21d28a5

SeanCurtis-TRI self-assigned this Jun 3, 2020

SeanCurtis-TRI reviewed Jun 3, 2020

View reviewed changes

Fixed linker errors

4df76bf

SeanCurtis-TRI approved these changes Jun 3, 2020

View reviewed changes

SeanCurtis-TRI requested changes Jun 3, 2020

View reviewed changes

Removed forward declarations for float templates

fa0921a

SeanCurtis-TRI requested changes Jun 4, 2020

View reviewed changes

SeanCurtis-TRI force-pushed the master branch from 65968c8 to 6599ed9 Compare June 4, 2020 15:01

SeanCurtis-TRI reviewed Jun 4, 2020

View reviewed changes

SeanCurtis-TRI reviewed Jun 15, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed issue with creating instances of type InterpMotion<float> #476

Fixed issue with creating instances of type InterpMotion<float> #476

shumi12321 commented Jun 3, 2020 •

edited by SeanCurtis-TRI

Loading

shumi12321 commented Jun 3, 2020

shumi12321 commented Jun 3, 2020

shumi12321 commented Jun 3, 2020

SeanCurtis-TRI left a comment

SeanCurtis-TRI left a comment

shumi12321 commented Jun 3, 2020

SeanCurtis-TRI left a comment

SeanCurtis-TRI left a comment

shumi12321 commented Jun 3, 2020

SeanCurtis-TRI left a comment

shumi12321 commented Jun 4, 2020

shumi12321 commented Jun 4, 2020

shumi12321 commented Jun 4, 2020

shumi12321 commented Jun 4, 2020

shumi12321 commented Jun 4, 2020

shumi12321 commented Jun 4, 2020

shumi12321 commented Jun 4, 2020

shumi12321 commented Jun 4, 2020

shumi12321 commented Jun 4, 2020

shumi12321 commented Jun 4, 2020

shumi12321 commented Jun 4, 2020

shumi12321 commented Jun 4, 2020

shumi12321 commented Jun 4, 2020

SeanCurtis-TRI left a comment

SeanCurtis-TRI left a comment

sherm1 commented Jun 4, 2020

shumi12321 commented Jun 14, 2020

shumi12321 commented Jun 14, 2020

shumi12321 commented Jun 14, 2020

SeanCurtis-TRI left a comment

Fixed issue with creating instances of type InterpMotion<float> #476

Are you sure you want to change the base?

Fixed issue with creating instances of type InterpMotion<float> #476

Conversation

shumi12321 commented Jun 3, 2020 • edited by SeanCurtis-TRI Loading

shumi12321 commented Jun 3, 2020

shumi12321 commented Jun 3, 2020

shumi12321 commented Jun 3, 2020

SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

shumi12321 commented Jun 3, 2020

SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

shumi12321 commented Jun 3, 2020

SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

shumi12321 commented Jun 4, 2020

shumi12321 commented Jun 4, 2020

shumi12321 commented Jun 4, 2020

shumi12321 commented Jun 4, 2020

shumi12321 commented Jun 4, 2020

shumi12321 commented Jun 4, 2020

shumi12321 commented Jun 4, 2020

shumi12321 commented Jun 4, 2020

shumi12321 commented Jun 4, 2020

shumi12321 commented Jun 4, 2020

shumi12321 commented Jun 4, 2020

shumi12321 commented Jun 4, 2020

shumi12321 commented Jun 4, 2020

SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

sherm1 commented Jun 4, 2020

shumi12321 commented Jun 14, 2020

shumi12321 commented Jun 14, 2020

shumi12321 commented Jun 14, 2020

SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

shumi12321 commented Jun 3, 2020 •

edited by SeanCurtis-TRI

Loading