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

Rename Resolution to Mag #8111

Merged
merged 45 commits into from
Oct 21, 2024
Merged

Rename Resolution to Mag #8111

merged 45 commits into from
Oct 21, 2024

Conversation

knollengewaechs
Copy link
Contributor

@knollengewaechs knollengewaechs commented Oct 5, 2024

In this PR, the term "resolution" is replaced with "mag"/"magnification" in a lot of places.

  • For the backend, see this comment below. The DB schema was adjusted, too.
  • In the frontend code, method names, their parameters, types, properties, error messages and console prints were adjusted. Function or local scoped variables were not adjusted, because this would make the changes way too large to review. As I am not familiar with the shader code, I didnt change *.glsl.ts files either.
  • In the UI, the term "resolution" is only being used in the context of animations after this PR. Other than that, "magnification" or sometimes "mag" is (or should be) used in toasts, tooltips, modals, sliders etc.

URL of deployed dev instance (used for testing):

Steps to test:

  • Test that everythings works as before by trying to test features that depend on mags.
  • This includes:
    • Opening an annotation, zooming in and out
    • Creating an annotation with restricted mags
    • Creating a volume layer with restricted mags
    • Testing all tools in the toolbar
    • Create & Edit task type with magRestrictions, create task + open it, should only have the allowed mags
    • reload dataset list from disk

TODOs:

  • backend
  • frontend
  • rename files, too
  • fix frontend tests
  • fix e2e tests
  • add some explanation to info tab tooltip

backup branch: https://github.com/scalableminds/webknossos/tree/backup-rename-resolution-to-mag

Issues:


@knollengewaechs knollengewaechs self-assigned this Oct 5, 2024
@fm3
Copy link
Member

fm3 commented Oct 8, 2024

@dieknolle3333 I pushed a commit changing a lot of usages in the backend. This changes the protocol in a couple of ways (please adapt the frontend):

  • resolutionRestrictions → magRestrictions (everywhere, e.g. tasktype json, AnnotationLayerParameters. just search for all occurrences)
  • volumeTracing proto → resolutions → mags
  • skeletonTracing proto → node.resolution → node.mag

There are also a couple of spots where a change might be expected but cannot be easily done without breaking backwards compatibility with hard-to-migrate data. I did not change:

  • CreateNodeSkeletonUpdateAction.resolution
  • DataLayer.resolutions (this is a big one. we need to get rid of the datasource-properties.json eventually)
  • WKWDataLayer.wkwResolutions

I also did not yet change the postgres tables. I might add this later if I manage to find time. That should not have impacts on the frontend-backend protocol though. For me to test that I don’t break anything there, it would be nice if everything else worked again first.

Please ping me again if anything seems out of place. I might have missed some spots of course.

@knollengewaechs
Copy link
Contributor Author

thank you so much! 🙏 I will get back to you once I am done with the frontend or if I have a question about the backend part.

@knollengewaechs
Copy link
Contributor Author

knollengewaechs commented Oct 10, 2024

after trying to rename "resolution" in a lot of places, I came to the conclusion that I will have to narrow the scope. I will rename the most important types and methods, but not do it everywhere. This also means that I will probably rebase.

@knollengewaechs
Copy link
Contributor Author

knollengewaechs commented Oct 14, 2024

@fm3 from my side, the first iteration for the frontend is done. I renamed most methods and most parameters and added some sentences to the UI.
Do you want to adjust the DB tables?

@fm3
Copy link
Member

fm3 commented Oct 14, 2024

Did that now :) From a first glance, it looks like things still work fine 🙌

Could you elaborate a bit on how you narrowed the scope? What areas/things did you skip? I guess this is related to the things I didn’t want to change in the backend as listed above?

@fm3 fm3 marked this pull request as ready for review October 14, 2024 13:55
@fm3
Copy link
Member

fm3 commented Oct 14, 2024

I fixed the snapshot tests, added a changelog entry and slightly updated the PR description. Once you described the scope as I mentioned above, this should be ready for a first review round :)

Copy link
Contributor Author

@knollengewaechs knollengewaechs left a comment

Choose a reason for hiding this comment

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

@fm3 or @MichaelBuessemeyer: there were merge conflicts in the migrations because the worker name already had no. 121. So I gave the migration in this PR no. 122 and changed it in MIGRATIONS.unreleased.md, and renamed the files in conf/evolutions/122-resolution-to-mag.sql and /reversions/. I hope this was correct!
edit: nvm, the pipeline turned red

@fm3
Copy link
Member

fm3 commented Oct 16, 2024

Yes, thanks! I see you found the various pitfalls of schema versions 😅 I added a commit also adapting the sql reversion (It is not checked in the CI, but could become important if we need to roll back after deploying this)

Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer left a comment

Choose a reason for hiding this comment

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

Nice, thanks you two :)

I didn't do another "extensive" testing session as only local vars were changed.

But sadly I noticed that the mag restrictions setting of task types is now buggy on this branch. @dieknolle3333 could you please fix this before merging? I just identified a potential reason for this.

How to test / reproduce:
1.

  • create a new task type with mag restrictions (e.g. min 1, max 2)
  • edit this task type -> the mag restriction setting should be empty -> broken
  • create a task with this task type
  • open such a task and try to zoom out of the valid range. On the master a warning toast is shown, but on this branch no toast is shown.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hej, I just noticed that the following messages aren't used in the code base

  • dataset.mag_mismatch
  • tracing.volume_mag_mismatch

Could you maybe double check this and in case you have the same result remove them? I'd say there is no use in keeping and maintaining them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, they are not used. I also noticed this. I'll remove them

Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer left a comment

Choose a reason for hiding this comment

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

Awesome thanks 🙏

The fix works and thanks for removing the unused messages

@knollengewaechs knollengewaechs enabled auto-merge (squash) October 21, 2024 14:09
@knollengewaechs knollengewaechs merged commit 60f77ac into master Oct 21, 2024
2 checks passed
@knollengewaechs knollengewaechs deleted the mag-all-the-way branch October 21, 2024 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants