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

Consider Incorporating FarseerDuality Project Into Main Package / Solution #570

Closed
ilexp opened this issue Oct 3, 2017 · 9 comments
Closed
Labels
Breaking Change Breaks binary or source compatibility Core Area: Duality runtime or launcher Dependency Area: External dependency of the project Task ToDo that's neither a Bug, nor a Feature
Milestone

Comments

@ilexp
Copy link
Member

ilexp commented Oct 3, 2017

Summary

Duality uses a custom Farseer Physics port, which is built and deployed separately. This not only increases iteration times heavily, but it also makes Duality-specific specialization harder. Consider moving the FarseerDuality project into main and renaming it to Duality.Physics, paving the way for gradually improving and streamlining it for integrated Duality usage.

Analysis

  • Reasoning behind moving the port closer to Duality and away from its origin:
    • It's based on an old Farseer version (3.3.1) and updating to the head version is unlikely because the gain / stability tradeoff doesn't sound great with regard to unresolved issues in the latest stable (3.5) of the original project which could easily turn out to be critical.
    • Farseer will no longer be updated and Velcro will take a bit until reaching stability, so a switch is unlikely at this point.
    • The custom Duality port of Farseer has already deviated from the original a lot.
    • It has also proven itself to be very stable over the past years.
    • With the above in mind, further deviation and specialization towards Duality might be beneficial.
    • As there will be no direct merge between core and physics library, switching to a different physics library internally will always remain a possibility.
  • The project should probably be located in Source/Core/Physics next to Primitives and Duality.
    • Could consider renaming the folders to Duality.Physics and Duality.Primitives, but should apply the same naming convention to the Source/Editor folders as well, making them Duality.Editor and Duality.Updater.
    • This would prompt renaming their project files as well. Lots of renames, but then project names, package names and namespaces would be aligned.
  • Even when moving it to the main repository, physics should remain a separate project.
  • Efficiency / performance improvement akin to Ideas For a High-Performance System Design Genbox/VelcroPhysics#29 can be considered in future iterations.
@ilexp ilexp added Breaking Change Breaks binary or source compatibility Core Area: Duality runtime or launcher Dependency Area: External dependency of the project Task ToDo that's neither a Bug, nor a Feature labels Oct 3, 2017
@ilexp ilexp added this to the v3.0 milestone Oct 3, 2017
@Jared-Miller
Copy link
Contributor

Started merging the Farseer code into the main Duality project. For now, I left all the package names as is and put the Farseer code into Source/Core/Physics named DualityPhysics, to match the way primitives was structured.

@ilexp

  • How would you prefer to handle the assembly information for the merged project?
  • Do you want the global .editorconfig styling changes applied to the DualityPhysics project?

@ilexp
Copy link
Member Author

ilexp commented Jan 2, 2018

How would you prefer to handle the assembly information for the merged project?

I'm not sure I understand the question, but the goal would be that the DualityPhysics project is later released as its own core dependency NuGet package similar to DualityPrimitives.

Do you want the global .editorconfig styling changes applied to the DualityPhysics project?

Yes, but not necessarily in the first iteration. I would be fine if this was done separately after this issue is resolved, or right before closing it.


As far as I could see in a quick branch comparison, it seems like you based your branch off of master - however, as we're dealing with breaking changes here, we can't release this update as a regular v2.x update. Instead, please branch off of develop-3.0 in order to deal with this issue.

Also (assuming that you're new to the project) be aware that this issue is not tagged as Help Wanted mainly because I expect complications and structural decisions that are easier done when bringing along deeper knowledge of the project and its internals. It might not be an ideal choice for a first contribution. If you're looking for a good one to start, besides the Help Wanted tag also look out for those tagged with Good First Issue - we don't always have one of those waiting, but they're ideal starters.

In any case, let me know if you want to continue work on this. I definitely do not want to discourage your efforts and I really appreciate you want to contribute, but I also want to make sure new contributors get a proper learning curve and don't too many walls in their first task.

@Jared-Miller
Copy link
Contributor

I'm not sure I understand the question, but the goal would be that the DualityPhysics project is later released as its own core dependency NuGet package similar to DualityPrimitives.

Sorry, I was wondering if you would want the assembly information to be changed (title from Farseer Physics Duality to Duality Physics, version from 4.1.4 to 3.x.x, etc). Based on your response, that appears to be what you were thinking. I've modified the AssemblyInfo and nuspec information to match what was done in primitives.

As far as I could see in a quick branch comparison, it seems like you based your branch off of master - however, as we're dealing with breaking changes here, we can't release this update as a regular v2.x update. Instead, please branch off of develop-3.0 in order to deal with this issue.

Yes, that should've been from develop-3.0. Changed the base.

In any case, let me know if you want to continue work on this. I definitely do not want to discourage your efforts and I really appreciate you want to contribute, but I also want to make sure new contributors get a proper learning curve and don't too many walls in their first task.

I don't mind some walls. I saw this in the list of 3.0 milestone items and it seemed like the upfront work was relatively simple (moving and renaming files, projects, and packages). Once that's completed, any other issues being blocked by the separation of the projects could be completed in 3.0.

@ilexp
Copy link
Member Author

ilexp commented Jan 2, 2018

Sorry, I was wondering if you would want the assembly information to be changed (title from Farseer Physics Duality to Duality Physics, version from 4.1.4 to 3.x.x, etc). Based on your response, that appears to be what you were thinking. I've modified the AssemblyInfo and nuspec information to match what was done in primitives.

Ah. Yes, that was the plan. Now that you mention it though, we will need to make sure it is absolutely clear that this package / library was originally based on FarseerPhysics.

Yes, that should've been from develop-3.0. Changed the base.

👍

I don't mind some walls. I saw this in the list of 3.0 milestone items and it seemed like the upfront work was relatively simple (moving and renaming files, projects, and packages). Once that's completed, any other issues being blocked by the separation of the projects could be completed in 3.0.

Alright then 🙂 Let me know how things are going and whether you need more information on anything. Also, feel free to open up a PR for this early if you need a review.

@Jared-Miller
Copy link
Contributor

Now that you mention it though, we will need to make sure it is absolutely clear that this package / library was originally based on FarseerPhysics.

That was the concern. I don't know if any of the information in the assembly itself is appropriate for that, though. Each of the source files still contains the original notice and the license information from FarseerDuality will be in the Physics directory. The nuspec definition currently indicates that it is a custom version of Farseer. Perhaps that is the best place to indicate that it is a fork, using the title, description, and/or summary?

@ilexp
Copy link
Member Author

ilexp commented Jan 2, 2018

That was the concern. I don't know if any of the information in the assembly itself is appropriate for that, though. Each of the source files still contains the original notice and the license information from FarseerDuality will be in the Physics directory.

We should definitely keep the copyright and license notice in the subfolder that contains the project. Maybe add a local readme.md too, where we explain that this is still a custom Farseer variant.

The nuspec definition currently indicates that it is a custom version of Farseer. Perhaps that is the best place to indicate that it is a fork, using the title, description, and/or summary?

Mentioning it in both the description and summary sounds good. Keep it short for the summary, maybe add as a separate paragraph in the description.

@Jared-Miller
Copy link
Contributor

Jared-Miller commented Jan 2, 2018

Maybe add a local readme.md too, where we explain that this is still a custom Farseer variant.

There was already a readme there, so I'll update it to include the new name (Duality.Physics) and keep the reference to the original repository.

I'll start renaming the folders in Source/Core and Source/Editor tomorrow, if you haven't changed your mind on that. Was there anything I hadn't listed explicitly in the PR that you believe should be part of this issue resolution?

@ilexp
Copy link
Member Author

ilexp commented Jan 3, 2018

I'll start renaming the folders in Source/Core and Source/Editor tomorrow, if you haven't changed your mind on that.

Please skip that for now - in fact, I hadn't made up my mind about them in the first place, and I'm not quite sure it would be a good idea. It was more a general thought that could be explored, rather than a work item or task.

Was there anything I hadn't listed explicitly in the PR that you believe should be part of this issue resolution?

Will get back to you on that as soon as I find time to review your PR progress :)

ilexp added a commit that referenced this issue Jan 4, 2018
Incorporate FarseerDuality into Main Solution (#570)
@ilexp
Copy link
Member Author

ilexp commented Jan 4, 2018

Addressed by @Jared-Miller in PR #596. Closing this.

@ilexp ilexp closed this as completed Jan 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Breaks binary or source compatibility Core Area: Duality runtime or launcher Dependency Area: External dependency of the project Task ToDo that's neither a Bug, nor a Feature
Development

No branches or pull requests

2 participants