#171 Creating a Line Shape

[ryanngu023]

This is mainly a draft PR to hopefully get some feedback on our progress on this issue. So far, we have been able to get Wordplay to recognize line as a function, have it be rendered upon the canvas and in the correct positions according to user input, differentiate itself from a rectangle, and still have the qualities of a shape. Our work can mainly be found in ShapeView.svelte, OutputTexts.ts, locale/en-US.json, Form.ts, Physics.ts, Shape.ts, createDefaultShares.ts, and schemas/Locale.json.

However, we have run into three main issues which are:

• We fail some of the tests in FormattdLiteral.test.ts and FormatedLiteral.test.ts, which mainly involve checking the translation between locales. We are currently unsure of why this occurs. (Maybe placeholder line documentation in other languages?)
• The physics of the line no longer works, meaning that other objects such as Phrases, that has matter attached to it will no longer interact with the line within the canvas. We are currently working on fixing this (Most likely need to relook at Matter.js and our previous work in Physics.ts as it may not work with our current way of rendering).
• How we differentiate between Rectangle and Line is not best practice, as we currently convert it to wordplay and then check if it includes either rectangle or line. We have a decent idea on how to fix this as we found out during meeting that a potential change could be checking the value of the output (we would have to implement a workaround to avoid checking a shape output to a shape output which would end up being the same).

Sorry if our branch is a little messy, but we would appreciate any kind of feedback that can be provided! Also let us know if there are any other major issues with the branch (I know that our coding style doesn't pass npm run check, but some of them seem to come from files that we didn't really touch).

[amyjko]

Great progress! Lots to talk about here:

• The branch includes many commits that do not have anything to do with the line feature. Definitely clean these up; the only changes in this branch should be the changes you've made for the line feature, not other commits from main. Best practice is to rebase off of main frequently and resolve conflicts as you go, per our process docs. It looks like you may not have been doing that, and there's a mix of changes from old versions of main, which is why so many tests are failing. If you're feeling like you're branch is too messy to clean up, you can always create a fresh one off of main, and incorporate your edits manually. Everything will be much harder to debug and understand with all of this other changes haphazardly included in the branch.

• Fix all TypeScript errors before proceeding. Those are almost always defects and can contribute to failing tests, and the failures you're describing.

• For the physics case you mention, please include a test case so that I can help you debug it. (Just as you would with a bug report).

• For the Line vs Rectangle differentiation, please indicate where you're doing this so I can review.

Once you've cleaned up the branch and shared the information above, I can review. (It's very hard to review otherwise, since there are so many other unrelated issues).

Thanks!

[amyjko]

Great progress! Lots to talk about here:

The branch includes many commits that do not have anything to do with the line feature. Definitely clean these up; the only changes in this branch should be the changes you've made for the line feature, not other commits from main. Best practice is to rebase off of main frequently and resolve conflicts as you go, per our process docs. It looks like you may not have been doing that, and there's a mix of changes from old versions of main, which is why so many tests are failing. If you're feeling like you're branch is too messy to clean up, you can always create a fresh one off of main, and incorporate your edits manually. Everything will be much harder to debug and understand with all of this other changes haphazardly included in the branch.

Fix all TypeScript errors before proceeding. Those are almost always defects and can contribute to failing tests, and the failures you're describing.

For the physics case you mention, please include a test case so that I can help you debug it. (Just as you would with a bug report).

For the Line vs Rectangle differentiation, please indicate where you're doing this so I can review.

Once you've cleaned up the branch and shared the information above, I can review. (It's very hard to review otherwise, since there are so many other unrelated issues).

Thanks!

[amyjko]

@kaiden-ong @ryanngu023 What is the status of this PR? I had some feedback above, but I didn't see a reply. Will you be finishing this?

[ryanngu023]

Sorry for the late reply, but we plan on implementing the changes that you have requested so that our changes will be ready for review! I believe our current plan is to continue the work on this branch and getting it ready for review during spring quarter.

[amyjko]

@ryanngu023 Any updates on this branch? I haven't touched it since you mentioned you'd be working it this quarter, but I don't think I've seen any progress on it. Please post an update.

[ryanngu023]

Hi, sorry for the inactivity on this branch, this quarter got surprisingly hectic alongside external factors, so we unfortunately unable to work on it as much as we wished to. I don't think that we are able to continue working on this issue, however because this feature is fairly close to done, added with the new references and related features of circle and polygon (although their implementations slightly clash with line), I will provide as much information about our changes and the work that needs to be done for any future developers working on this issue.

What we have done overall:

• In ShapeView.svelte, we have added logic to calculate the angle and length of the line, and display it to the user using conditional rendering
• We have added the relevant documents and locales in OutputTexts.ts, en-US.json, and Locale.json
• We have created the relevant wrapper class and structure creation functions for line in Form.ts (although this clashes with the new toForm function)
• We currently have a way to differentiate between a line and other shapes in the toShape function within Shape.ts but this is outdated due to the new toForm function
• We have a createLine function in Physics.ts which creates the collision for the line, however this does not fully work and is outdated with how circle and polygon's collisions are created.

Most of the steps are complete, from writing the locales and documentation, to most of the rendering steps. I believe the only steps after this is to fix the Physics for line, and adjust the code so that it is compatible with the new methods of rendering shapes. I believe the major steps that need to be done before this feature can be implemented is:

• Add the toLine function to the toForm function in Form.ts, so that way it would render with the new updates and would also fix our previous issue of having trouble differentiating a line from other shapes.
• Modify our createLine and line physics code to follow and be similar to the circle and polygon physics code
• Adjust the way we implemented the front end of the line in ShapeView.svelte to match the other shapes such as circle and polygon (basically just add another conditional rendering for line)

I attempted to rebase our branch and also add testing, but due to the large amount of irrelevant commits, it introduced a lot of strange bugs and broke the project in different ways I couldn't figure out. I think the best way is to look through these files ShapeView.svelte, OutputTexts.ts, locale/en-US.json, Form.ts, Physics.ts, Shape.ts, createDefaultShares.ts, and schemas/Locale.json and look through any code related to 'Line'. (easiest way is probably ctrl+f the word 'line' in these files).

I will be unassigning myself from this issue, but will still be available on discord @RyanNg or through github if further information, steps or any questions are raised on this issue. Again, I'm sorry for the lack of communication and progress on this issue, I really wished I could have completed this feature.

[amyjko]

Thank you for the update @ryanngu023, and your contributions. I'll try to work through this at some point, and see if I can salvage the work.