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

Adapting Vessel.js to ES6 class #165

Merged
merged 32 commits into from
Nov 26, 2022
Merged

Adapting Vessel.js to ES6 class #165

merged 32 commits into from
Nov 26, 2022

Conversation

ferrari212
Copy link
Contributor

No description provided.

* Add a web-page for testing modules.
* Adjust JSONSpec style.
* Fix export/import syntax and add default modules.
@icarofonseca icarofonseca marked this pull request as draft September 5, 2021 19:53
@icarofonseca
Copy link
Member

icarofonseca commented Sep 5, 2021

Our first goal might be to convert DerivedObject, Hull, Structure and ShipState to the new convention.

Then we can try to rebuild this app with modules.

@icarofonseca
Copy link
Member

As far as I understood Classes syntax, there were some mistakes on the BaseObject.js. I am using the following link for reference: mrdoob/three.js#19980 (comment)

@ferrari212
Copy link
Contributor Author

As far as I understood Classes syntax, there were some mistakes on the BaseObject.js. I am using the following link for reference: mrdoob/three.js#19980 (comment)

Good references Ícaro. This gives handy tricks to exchange properly from function to class.

@ferrari212
Copy link
Contributor Author

ferrari212 commented Nov 6, 2021

I created a test folder which contains possible test files we can start using for now on. I created a playground.js script which contains the ship version instantiated by the two forms. The idea is to compare the two methods to ensure both of them present the same results.

@ferrari212
Copy link
Contributor Author

ferrari212 commented Nov 28, 2021

I created a project Kanban to track which of the elements are already modified to avoid double work. In addtion, I already finnshed recreating the most crucial modules as agreed in the comment above:

Our first goal might be to convert DerivedObject, Hull, Structure and ShipState to the new convention.

Then we can try to rebuild this app with modules.

With that accomplishement the app stated can be recreated. I assume it fits best inside the folder tester. In addition, be aware I am testing each one of the modules in the ./tests/playground.html.

@ferrari212
Copy link
Contributor Author

With the completion of the commit 2e9dea6 we reached the milestone defined in #165 (comment). Check the tests/playground.html to see the results.

@ferrari212
Copy link
Contributor Author

The new version using class were not behaving as intend and was retriving undefined in the new class style instead of the value expected for the volume in this line:

V2 = this.calculateAttributesAtDraft(t)["Vs"] - VT;

As solution, I modified the method which were returning a function to return the final object instead. The results seems ok for the tests done so far:

calculateAttributesAtDraft( T ) {

For a comparison here is the function as it was before:

calculateAttributesAtDraft: function() {

I do not know why this function was designed that way in the first place and I am not sure about the implications in terms of efficiency or applicability of this modification, therefore, I am making this comment in case someone has better idea how to handle this specific part of the code.

@ferrari212 ferrari212 marked this pull request as ready for review October 9, 2022 15:57
Copy link
Member

@icarofonseca icarofonseca left a comment

Choose a reason for hiding this comment

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

Merge the PR to dev so it is possible to start with more thorough tests of examples.

@icarofonseca icarofonseca merged commit 4cf0eca into shiplab:dev Nov 26, 2022
ferrari212 added a commit to ferrari212/vesseljs that referenced this pull request Dec 4, 2022
Merge pull request shiplab#165 from ferrari212/master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants