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.

Sign up for GitHub

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

Jump to bottom

Rename isosurface to adHocMesh #7350

Merged
merged 12 commits into from
Oct 16, 2023
Merged

Rename isosurface to adHocMesh #7350

merged 12 commits into from
Oct 16, 2023

Conversation

frcroth
Copy link
Member

@frcroth frcroth commented Sep 19, 2023 •
edited

URL of deployed dev instance (used for testing):

  • https://___.webknossos.xyz

Issues:

(Please delete unneeded items, merge only when none are left open)

@frcroth
Copy link
Member Author

frcroth commented Oct 9, 2023 •
edited

@philippotto I have also done the refactoring in the frontend now, there is a CI error which I don't understand.
I tested creating an adhoc mesh once and it seemed to work, but something else might be broken, since this refactoring was done without understanding what was refactored.

hotzenklotz
hotzenklotz reviewed Oct 9, 2023
View reviewed changes
Copy link
Member

@hotzenklotz hotzenklotz left a comment

Choose a reason for hiding this comment

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

@philippotto Three questions for the frontend part:

  1. Do you know if there is a difference in terminology between mesh (pre-computed) and mesh (ex isosurface) that will be blurred by this renaming?
  2. Are there any caches/local storage that rely on this naming? (object property name / keys)
  3. What about URL encoding of the selected meshes? Does this reference "isosurface"?

frontend/javascripts/admin/admin_rest_api.ts Outdated Show resolved Hide resolved
frontend/javascripts/admin/admin_rest_api.ts Outdated Show resolved Hide resolved
@philippotto
Copy link
Member

@frcroth Very cool to this this clean up 👍

@hotzenklotz You ask good question. Luckily, we should be safe on all these points, though.

  1. Do you know if there is a difference in terminology between mesh (pre-computed) and mesh (ex isosurface) that will be blurred by this renaming?

No, I think, this should be fine.

  1. Are there any caches/local storage that rely on this naming? (object property name / keys)

No, I don't think so. Should be fine.

  1. What about URL encoding of the selected meshes? Does this reference "isosurface"?

I just checked this and this uses a boolean precomputed property. So, no issues here.

The PR itself looks good. Will test now.

philippotto
philippotto reviewed Oct 11, 2023
View reviewed changes
Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

Testing went well, too 👍

@frcroth Please update the changelog and mark the PR as ready 🚢

@frcroth frcroth requested a review from fm3 October 16, 2023 08:45
@frcroth frcroth marked this pull request as ready for review October 16, 2023 08:47
fm3
fm3 reviewed Oct 16, 2023
View reviewed changes
Copy link
Member

@fm3 fm3 left a comment

Choose a reason for hiding this comment

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

Great! I really enjoy how we’re moving towards more consistent terminology.

MIGRATIONS.unreleased.md Outdated Show resolved Hide resolved
...-datastore/app/com/scalableminds/webknossos/datastore/controllers/BinaryDataController.scala Outdated Show resolved Hide resolved
...ssos-datastore/app/com/scalableminds/webknossos/datastore/services/AdHocMeshingService.scala Outdated
actorSystem: ActorSystem,
isosurfaceTimeout: FiniteDuration,
isosurfaceActorPoolSize: Int)(implicit ec: ExecutionContext)
class AdHocMeshingService(binaryDataService: BinaryDataService,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
class AdHocMeshingService(binaryDataService: BinaryDataService,
class AdHocMeshService(binaryDataService: BinaryDataService,

I think I’d prefer it without the “ing” since that would be the only place where the service would be named with a conjucated form.

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed all ings

@frcroth frcroth changed the title WIP: Rename isosurface in to adHocMesh Rename isosurface in to adHocMesh Oct 16, 2023
@frcroth frcroth requested a review from fm3 October 16, 2023 14:24
@frcroth frcroth changed the title Rename isosurface in to adHocMesh Rename isosurface to adHocMesh Oct 16, 2023
@frcroth frcroth merged commit 08852e8 into master Oct 16, 2023
2 checks passed
@frcroth frcroth deleted the isosurface-to-adhocmesh branch October 16, 2023 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hotzenklotz hotzenklotz hotzenklotz left review comments

@philippotto philippotto philippotto left review comments

@fm3 fm3 fm3 approved these changes

Assignees

@frcroth frcroth

Labels
backend frontend
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Rename “isosurface“ to “adHocMesh” in code
4 participants
@frcroth @philippotto @hotzenklotz @fm3