WeSign/unity-application
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases 6 Wiki Activity

Resolve WES-127-Refactor_Themelist #69

Merged
jrcoudro merged 8 commits from WES-127-Refactor-ThemeList into development 2023-03-30 21:10:42 +00:00
Conversation 28 Commits 8 Files Changed 26 +261 -15
jrcoudro commented 2023-03-27 08:26:19 +00:00 (Migrated from gitlab.ilabt.imec.be)
Copy Link

Description

  • Seperated the models from the themes via the use of an index, this allows us to change the models in one spot only.
  • Changed controller-scripts to make use of this new ModelIndex.
  • Changed SignPredictor to not use a given model, instead it now uses the CurrentModel as given by the ModelList.

Testing Instructions

Please provide instructions on how the code reviewers can test your changes:

  1. Check if minigames still work as intended.
  2. Check if courses still work as intended (the preview as well).
  3. Check the theme-script, modelList-script and SignPredictor-script to see if you agree with the design-choice.

Related Issues

WES-131 : The modelList brings the model closer to the signpredictor, which is handy for the SignPredictor-refactor

Checklist

  • I have filled in this template.
  • I have tested my changes thoroughly (both in the editor + build and run (ctrl+B)!).
  • I have added appropriate unit tests.
  • I have updated the user documentation as necessary.
  • Code reviewed by 2 people.

Closes WES-127

## Description - Seperated the models from the themes via the use of an index, this allows us to change the models in one spot only. - Changed controller-scripts to make use of this new ModelIndex. - Changed SignPredictor to not use a given model, instead it now uses the CurrentModel as given by the ModelList. ## Testing Instructions _Please provide instructions on how the code reviewers can test your changes:_ 1. Check if minigames still work as intended. 2. Check if courses still work as intended (the preview as well). 3. Check the theme-script, modelList-script and SignPredictor-script to see if you agree with the design-choice. ## Related Issues WES-131 : The modelList brings the model closer to the signpredictor, which is handy for the SignPredictor-refactor ## Checklist - [x] I have filled in this template. - [x] I have tested my changes thoroughly (both in the editor + **build and run (ctrl+B)**!). - [ ] I have added appropriate unit tests. - [x] I have updated the user documentation as necessary. - [ ] Code reviewed by 2 people. Closes WES-127
jrcoudro commented 2023-03-27 08:26:19 +00:00 (Migrated from gitlab.ilabt.imec.be)
Copy Link

requested review from @louadria, @tihabils, @dvschuyl, @lvrossem, @hvbreuge, @jeldgees, and @jrcoudro

requested review from @louadria, @tihabils, @dvschuyl, @lvrossem, @hvbreuge, @jeldgees, and @jrcoudro
jeldgees (Migrated from gitlab.ilabt.imec.be) approved these changes 2023-03-27 08:26:19 +00:00
louadria (Migrated from gitlab.ilabt.imec.be) approved these changes 2023-03-27 08:26:19 +00:00
jrcoudro commented 2023-03-27 08:57:35 +00:00 (Migrated from gitlab.ilabt.imec.be)
Copy Link

added 1 commit

  • be745723 - Added test-code and moved scripts to correct folders

Compare with previous version

added 1 commit <ul><li>be745723 - Added test-code and moved scripts to correct folders</li></ul> [Compare with previous version](/wesign/unity-application/-/merge_requests/69/diffs?diff_id=32819&start_sha=78f4d961f71e36841374e4f171d666ede1114f7a)
jrcoudro commented 2023-03-27 08:58:00 +00:00 (Migrated from gitlab.ilabt.imec.be)
Copy Link

marked this merge request as ready

marked this merge request as **ready**
jrcoudro commented 2023-03-27 08:58:00 +00:00 (Migrated from gitlab.ilabt.imec.be)
Copy Link

changed title from {-Draft: -}Resolve WES-127-R{-E-}factor_Themelist to Resolve WES-127-R{+e+}factor_Themelist

changed title from **{-Draft: -}Resolve WES-127-R{-E-}factor_Themelist** to **Resolve WES-127-R{+e+}factor_Themelist**
jrcoudro commented 2023-03-27 09:00:20 +00:00 (Migrated from gitlab.ilabt.imec.be)
Copy Link

changed the description

changed the description
jeldgees commented 2023-03-27 12:28:51 +00:00 (Migrated from gitlab.ilabt.imec.be)
Copy Link

approved this merge request

approved this merge request
louadria commented 2023-03-27 20:45:35 +00:00 (Migrated from gitlab.ilabt.imec.be)
Copy Link

Shouldn't you use the GetModelbyIndex function here?

Shouldn't you use the GetModelbyIndex function here?
louadria commented 2023-03-27 20:50:04 +00:00 (Migrated from gitlab.ilabt.imec.be)
Copy Link

why not:
modelList.GetModelByIndex(...)...

why not: modelList.GetModelByIndex(...)...
louadria commented 2023-03-27 20:53:23 +00:00 (Migrated from gitlab.ilabt.imec.be)
Copy Link

It doesn't really matter, but you make a getter:
image
And then don't use it. Right now, it's only being used in your unit tests.

It doesn't really matter, but you make a getter: ![image](/uploads/3afa70599a04f9bc7b0b54a3ed889963/image.png) And then don't use it. Right now, it's only being used in your unit tests.
jrcoudro commented 2023-03-28 05:45:50 +00:00 (Migrated from gitlab.ilabt.imec.be)
Copy Link

Thats fair, I just kinda fogot them for some reason. Ill change it

Thats fair, I just kinda fogot them for some reason. Ill change it
jrcoudro commented 2023-03-28 06:44:24 +00:00 (Migrated from gitlab.ilabt.imec.be)
Copy Link

Okay, I get why I didn't use this function now, it uses a different index. This is because I reused logic from MinigameList. The getModelByINdex serves no purpose here though, so I will change it

Okay, I get why I didn't use this function now, it uses a different index. This is because I reused logic from MinigameList. The getModelByINdex serves no purpose here though, so I will change it
jrcoudro commented 2023-03-28 06:52:10 +00:00 (Migrated from gitlab.ilabt.imec.be)
Copy Link

changed this line in version 3 of the diff

changed this line in [version 3 of the diff](/wesign/unity-application/-/merge_requests/69/diffs?diff_id=32985&start_sha=be7457236c87875e4bc1b7a7ab7f0303bc2bf6b7#f54d6ba316cb3104b8cf0c07ff0a036374749b99_213_213)
jrcoudro commented 2023-03-28 06:52:10 +00:00 (Migrated from gitlab.ilabt.imec.be)
Copy Link

resolved all threads

resolved all threads
jrcoudro commented 2023-03-28 06:52:11 +00:00 (Migrated from gitlab.ilabt.imec.be)
Copy Link

added 1 commit

  • dfdb2ab1 - Updated getter in ModelList and cleaned Assembly

Compare with previous version

added 1 commit <ul><li>dfdb2ab1 - Updated getter in ModelList and cleaned Assembly</li></ul> [Compare with previous version](/wesign/unity-application/-/merge_requests/69/diffs?diff_id=32985&start_sha=be7457236c87875e4bc1b7a7ab7f0303bc2bf6b7)
dvschuyl commented 2023-03-28 10:50:15 +00:00 (Migrated from gitlab.ilabt.imec.be)
Copy Link

changed the description

changed the description
dvschuyl commented 2023-03-28 11:05:34 +00:00 (Migrated from gitlab.ilabt.imec.be)
Copy Link

Good job!

Some small remarks:

  • ModelIndex is not documented
  • ModelTuple can be part of the ModelList class (make it private, so it is not visible to the outer scope)
  • In ModelList.GetCurrentModel() : you don't check whether the currentModelIndex is out of bounds, it just crashes instead of returning null. Have a look at the MinigameList to see how it's implemented over there.
  • In ModelListTest : Seperate the get and set in different test methods.
  • In ModelListTest : You do not have any tests for when the ModelList is empty.
Good job! ### Some small remarks: - ModelIndex is not documented - ModelTuple can be part of the ModelList class (make it private, so it is not visible to the outer scope) - In `ModelList.GetCurrentModel()` : you don't check whether the currentModelIndex is out of bounds, it just crashes instead of returning `null`. Have a look at the MinigameList to see how it's implemented over there. - In `ModelListTest` : Seperate the get and set in different test methods. - In `ModelListTest` : You do not have any tests for when the ModelList is empty.
jrcoudro commented 2023-03-28 13:36:05 +00:00 (Migrated from gitlab.ilabt.imec.be)
Copy Link

Fair remarks, I'll take them into consideration. I changed the Getter now to return the ModelTuple though as this makes your third point a lot easier to implement.

Fair remarks, I'll take them into consideration. I changed the Getter now to return the ModelTuple though as this makes your third point a lot easier to implement.
jrcoudro commented 2023-03-28 13:37:26 +00:00 (Migrated from gitlab.ilabt.imec.be)
Copy Link

added 1 commit

Compare with previous version

added 1 commit <ul><li>be8885a5 - Implemented MR feedback</li></ul> [Compare with previous version](/wesign/unity-application/-/merge_requests/69/diffs?diff_id=33024&start_sha=dfdb2ab10b68659829ebd9893627fab9238fb4d7)
dvschuyl commented 2023-03-28 20:08:56 +00:00 (Migrated from gitlab.ilabt.imec.be)
Copy Link

Why are you now returning a ModelTuple? The reason why you put it in the ModeList class, is because it handles interior stuff for the ModelList. Don't make this visible to the outside world. I would suggest changing your code as follows:

    public NNModel GetCurrentModel()
    {
        return models.Find(x => x.model == models[currentModelIndex].model)?.model;

more information can be found here

Why are you now returning a ModelTuple? The reason why you put it in the ModeList class, is because it handles interior stuff for the ModelList. Don't make this visible to the outside world. I would suggest changing your code as follows: ```suggestion:-3+0 public NNModel GetCurrentModel() { return models.Find(x => x.model == models[currentModelIndex].model)?.model; ``` more information can be found [here](https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/operators/member-access-operators#null-conditional-operators--and-)
jrcoudro commented 2023-03-29 11:24:42 +00:00 (Migrated from gitlab.ilabt.imec.be)
Copy Link

That is better indeed, I didnt know about the “?” Notation so I kind of struggled with demanding a model from an object that was possibly null. Ill change it

That is better indeed, I didnt know about the “?” Notation so I kind of struggled with demanding a model from an object that was possibly null. Ill change it
jrcoudro commented 2023-03-29 17:17:49 +00:00 (Migrated from gitlab.ilabt.imec.be)
Copy Link

changed this line in version 5 of the diff

changed this line in [version 5 of the diff](/wesign/unity-application/-/merge_requests/69/diffs?diff_id=33122&start_sha=be8885a508d4d3db9824de5d8e834ac8e704daff#3a8cb27947722ddbf1229c68e2659d17026ce77b_47_46)
jrcoudro commented 2023-03-29 17:17:50 +00:00 (Migrated from gitlab.ilabt.imec.be)
Copy Link

added 1 commit

  • a44532b9 - Changed GetCurrentModel to return the model instead of the tuple

Compare with previous version

added 1 commit <ul><li>a44532b9 - Changed GetCurrentModel to return the model instead of the tuple</li></ul> [Compare with previous version](/wesign/unity-application/-/merge_requests/69/diffs?diff_id=33122&start_sha=be8885a508d4d3db9824de5d8e834ac8e704daff)
louadria commented 2023-03-30 12:01:54 +00:00 (Migrated from gitlab.ilabt.imec.be)
Copy Link

approved this merge request

approved this merge request
louadria commented 2023-03-30 12:06:34 +00:00 (Migrated from gitlab.ilabt.imec.be)
Copy Link

Isn't it easier to do something like this:

if (currentModelIndex in models) {
        return models[currentModelIndex].model;
} else {
        return null;
}

Don't know about the exact syntax of in, but this would be more readable, no?

It's okay for me how it is now too, just a suggestion though.

Isn't it easier to do something like this: ``` if (currentModelIndex in models) { return models[currentModelIndex].model; } else { return null; } ``` Don't know about the exact syntax of in, but this would be more readable, no? It's okay for me how it is now too, just a suggestion though.
jeldgees commented 2023-03-30 21:09:59 +00:00 (Migrated from gitlab.ilabt.imec.be)
Copy Link

approved this merge request

approved this merge request
jeldgees commented 2023-03-30 21:10:41 +00:00 (Migrated from gitlab.ilabt.imec.be)
Copy Link

mentioned in commit b8cdcd128b

mentioned in commit b8cdcd128b7499abc5d1d6b6d7335fe2b18ed20b
Sign in to join this conversation.
Reviewers
No Reviewers
jeldgees
louadria
Labels
Clear labels
bug
enhancement
impact::high
impact::low
impact::minor
impact::moderate
impact::very low
probability::high
probability::low
probability::minor
probability::moderate
probability::very low
progress::busy
progress::done
progress::review
progress::todo
version::0.1
version::0.2
version::0.3
version::0.4
version::0.5
version::1.0
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
VictorMylle
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: WeSign/unity-application#69
Delete Branch "WES-127-Refactor-ThemeList"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?