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

Thesis of @anonym-HPI #458

Conversation

anonym-HPI
Copy link
Contributor

@anonym-HPI anonym-HPI commented Jun 19, 2022

Replaced by #523

TODO:

  • dataStructure to find (more) efficiently elements around a specific element (patient, personnel or material)
  • incremental updates instead of calculating every treatment of every patient every time from scratch
  • change how many patients personnel and material can treat
  • notarzt aura (TODO: make aura uninteractable and best would be behind patients) - Aura got removed instead
  • fix tests / write tests for spatialTree (?)
  • write migrations

benchmarking will be done outside of this PR, in this branch:
https://github.com/anonym-HPI/digital-fuesim-manv/tree/experimental/bachelors-thesis-marvin-benchmarking

@anonym-HPI anonym-HPI self-assigned this Jun 19, 2022
@Dassderdie
Copy link
Collaborator

@anonym-HPI FYI:

You could also try to add the quadtree to the state by serializing and deserializing it:

// retrieving the quadtree from the state:
const quadtree = quadtree(state.quadtreeData);
// updating the quadtree in the state
draftState.quadtreeData = quadtree.data();

You would need to make sure that:

  • the data in the quadtree is not mutated by it (I'm not 100% sure about this)
  • These operations are not too expensive (benchmarks)
  • data is a JSON object (no functions or relying on references)

@anonym-HPI anonym-HPI force-pushed the experimental/bachelors-thesis-marvin branch from fa61a42 to 1a074f0 Compare June 25, 2022 18:38
@anonym-HPI anonym-HPI force-pushed the experimental/bachelors-thesis-marvin branch from 1a074f0 to aae014d Compare June 25, 2022 18:39
@anonym-HPI anonym-HPI force-pushed the experimental/bachelors-thesis-marvin branch from f3cc010 to e3d6f47 Compare July 1, 2022 21:23
@anonym-HPI anonym-HPI force-pushed the experimental/bachelors-thesis-marvin branch from cca0a3b to 2624d8c Compare July 2, 2022 11:22
@anonym-HPI anonym-HPI force-pushed the experimental/bachelors-thesis-marvin branch from 57d6c81 to ebbbad2 Compare July 3, 2022 12:12
@anonym-HPI anonym-HPI force-pushed the experimental/bachelors-thesis-marvin branch from b6627ce to 45c1901 Compare July 16, 2022 14:15
Copy link
Collaborator

@Dassderdie Dassderdie left a comment

Choose a reason for hiding this comment

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

My second iteration over the changes here. I'll apply some of the straight forward stuff today.
The next big challenge will probably be the migrations...

backend/src/exercise/exercise-wrapper.ts Show resolved Hide resolved
backend/src/exercise/patient-ticking.ts Show resolved Hide resolved
shared/src/data/default-state/personnel-templates.ts Outdated Show resolved Hide resolved
shared/src/data/default-state/personnel-templates.ts Outdated Show resolved Hide resolved
shared/src/data/default-state/personnel-templates.ts Outdated Show resolved Hide resolved
shared/src/store/action-reducers/utils/spatial-elements.ts Outdated Show resolved Hide resolved
shared/src/store/action-reducers/utils/spatial-elements.ts Outdated Show resolved Hide resolved
* Rename `default` material to `standard`
* Change GwSan to have 4 big materials instead of 8 standard
* Change treatment ranges of big material and notarzt
* Change icon size of big material to be roughly 2 instead of 4 times the area of the standard one
Copy link
Collaborator

@Dassderdie Dassderdie left a comment

Choose a reason for hiding this comment

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

From my side, this is now (finally!) ready to be merged.

@ClFeSc
Copy link
Contributor

ClFeSc commented Sep 16, 2022

@Dassderdie Do you have an idea why the workflows won't run? Maybe it's safer to first merge these changes to a branch in hpi-sam/digitalfuesimmanv where Actions will run and then merge to dev from there?

@Dassderdie Dassderdie changed the base branch from dev to feature/improve-treatment-system September 17, 2022 12:17
@Dassderdie Dassderdie dismissed ClFeSc’s stale review September 17, 2022 12:19

Has all been addressed and a second review could be done on the new branch.

@Dassderdie
Copy link
Collaborator

@Dassderdie Do you have an idea why the workflows won't run? Maybe it's safer to first merge these changes to a branch in hpi-sam/digitalfuesimmanv where Actions will run and then merge to dev from there?

Probably the best strategy. -> This branch gets replaced by https://github.com/hpi-sam/digital-fuesim-manv/tree/feature/improve-treatment-system.

@Dassderdie Dassderdie merged commit 7c26857 into hpi-sam:feature/improve-treatment-system Sep 17, 2022
@anonym-HPI
Copy link
Contributor Author

@Dassderdie Do you have an idea why the workflows won't run? Maybe it's safer to first merge these changes to a branch in hpi-sam/digitalfuesimmanv where Actions will run and then merge to dev from there?

This could be, because not enough reviews approved it, maybe?

@Dassderdie
Copy link
Collaborator

@Dassderdie Do you have an idea why the workflows won't run? Maybe it's safer to first merge these changes to a branch in hpi-sam/digitalfuesimmanv where Actions will run and then merge to dev from there?

This could be, because not enough reviews approved it, maybe?

Actions always ran independent from the number of approving reviews.
Reviewers should also be able to see the results of the pipeline before approving an pull request.
Creating a new branch in this repo and merging in there also fixed this problem...

@anonym-HPI
Copy link
Contributor Author

anonym-HPI commented Sep 17, 2022

@Dassderdie Do you have an idea why the workflows won't run? Maybe it's safer to first merge these changes to a branch in hpi-sam/digitalfuesimmanv where Actions will run and then merge to dev from there?

This could be, because not enough reviews approved it, maybe?

Actions always ran independent from the number of approving reviews.
Reviewers should also be able to see the results of the pipeline before approving an pull request.
Creating a new branch in this repo and merging in there also fixed this problem...

I don't know what the default is in Github, but this mechanism is available (or something like that). Think about 10000 forks creating PRs and costing the repo owner money or taking away every free pipeline minute. Don't know if it could run on the forked pipeline or if it does. I saw this on another bigger repo (but then you have problems when secrets are used, as they will not be transfered to the fork), without some kind of approval (maybe some other kind of approval) or so, the pipeline didn't start.

But I get that a reviewer would like to review a PR that checks all automatic checks first.

@ClFeSc
Copy link
Contributor

ClFeSc commented Sep 17, 2022

@Dassderdie Do you have an idea why the workflows won't run? Maybe it's safer to first merge these changes to a branch in hpi-sam/digitalfuesimmanv where Actions will run and then merge to dev from there?

This could be, because not enough reviews approved it, maybe?

Actions always ran independent from the number of approving reviews.
Reviewers should also be able to see the results of the pipeline before approving an pull request.
Creating a new branch in this repo and merging in there also fixed this problem...

I don't know what the default is in Github, but this mechanism is available. Think about 10000 forks creating PRs and costing the repo owner money or taking away every free pipeline minute. Don't know if it could run on the forked pipeline or if it does. I saw this on another bigger repo (but then you have problems when secrets are used, as they will not be transfered to the fork), without some kind of approval (maybe some other kind of approval) or so, the pipeline didn't start.

You can specifically configure that. For this repo, an approval is required for Actions of PRs by first-time contributers (or something similar). I think the issue here would rather be something like that we use a secret in the workflow file that is not specified in the fork. But either way, this is solved with the new branch.

@anonym-HPI
Copy link
Contributor Author

@Dassderdie Do you have an idea why the workflows won't run? Maybe it's safer to first merge these changes to a branch in hpi-sam/digitalfuesimmanv where Actions will run and then merge to dev from there?

This could be, because not enough reviews approved it, maybe?

Actions always ran independent from the number of approving reviews.
Reviewers should also be able to see the results of the pipeline before approving an pull request.
Creating a new branch in this repo and merging in there also fixed this problem...

I don't know what the default is in Github, but this mechanism is available. Think about 10000 forks creating PRs and costing the repo owner money or taking away every free pipeline minute. Don't know if it could run on the forked pipeline or if it does. I saw this on another bigger repo (but then you have problems when secrets are used, as they will not be transfered to the fork), without some kind of approval (maybe some other kind of approval) or so, the pipeline didn't start.

You can specifically configure that. For this repo, an approval is required for Actions of PRs by first-time contributers (or something similar). I think the issue here would rather be something like that we use a secret in the workflow file that is not specified in the fork. But either way, this is solved with the new branch.

We can work on the new one, in the future something like that should be maybe mitigated (as this breaks a long PR with also a long review process into two)

@Dassderdie
Copy link
Collaborator

We can work on the new one, in the future something like that should be maybe mitigated (as this breaks a long PR with also a long review process into two)

The significantly bigger problem of this PR was that it was way too big. The commit history is also not very good, as the review processes often refactored many of the changes here. So maybe closing this PR and making a fresh start is a good thing, as this PR got very convoluted.
Another thing that should have probably been done is writing a changelog/description of what this PR actually changes. This PR doesn't only change the treatment system but also does smaller refactorings, e.g. changes the name of the male-patient.svg asset.

TLDR: There are more significant problems than creating a separate branch/PR for a fork...

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.

None yet

4 participants