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

fix: well layer: wellNameAtTop control is inverted #2432 #2433

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

nilscb
Copy link
Collaborator

@nilscb nilscb commented Feb 3, 2025

@nilscb nilscb linked an issue Feb 3, 2025 that may be closed by this pull request
@nilscb nilscb self-assigned this Feb 3, 2025
@nilscb nilscb added bug Something isn't working AspenTech Task owned by AspenTech map-component Issues related to the map component. labels Feb 3, 2025
const h = this.context.viewport.height;

const percentages = [50, 25, 75, 12.5, 87.5, 25, 75];

const n = well_xyz?.length ?? 2;
if (well_xyz && n >= 2) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add some comments to explain the algorithm (are a ref to original if it comes from a known algo)

Copy link
Collaborator

Choose a reason for hiding this comment

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

BTW, I don't get the logic. If I understand correctly, if the algo fail with the provided percentage, a hard-coded list of percentages completely unrelated to the requested one is used.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Please wait a bit with this code review. The Wellslayer.stories file is included by mistake

Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment is not related to story, but to wellsLayer.ts

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a regular sampling along the trajectory in search of a camera visible location. It is shuffled to minimize large jumps and to tend towards the middle.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What bothers me is that it does absolutely not take into account requested position.
I would use an approach to shift away from the requested position , not to ignore it

Copy link
Collaborator

Choose a reason for hiding this comment

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

What bothers me is that it does absolutely not take into account requested position.
I would use an approach to shift away from the requested position , not to ignore it

I agree, it could be better usability if the labels shifted more gradually from the requested position instead of popping up at a random position.

@nilscb nilscb force-pushed the WellClutterContinued branch from 6af52bb to 0537835 Compare February 3, 2025 15:10
@nilscb
Copy link
Collaborator Author

nilscb commented Feb 4, 2025

The auto positioning of lables is now default off and controlled by the well property "wellNameAutoPosition"

const h = this.context.viewport.height;

const percentages = [50, 25, 75, 12.5, 87.5, 25, 75];

const n = well_xyz?.length ?? 2;
if (well_xyz && n >= 2) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment is not related to story, but to wellsLayer.ts

/** It true label position will be auto calculated if possible to be inside view volume at all times.
* default false.
*/
wellNameAutoPosition: boolean;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would rename that to adjustWellNamePosition

const h = this.context.viewport.height;

const percentages = [50, 25, 75, 12.5, 87.5, 25, 75];

const n = well_xyz?.length ?? 2;
if (well_xyz && n >= 2) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What bothers me is that it does absolutely not take into account requested position.
I would use an approach to shift away from the requested position , not to ignore it

@hkfb
Copy link
Collaborator

hkfb commented Feb 6, 2025

If I disable hideOverlappingWellNames then all labels disappear, which is unexpected:
image

Copy link
Collaborator

@hkfb hkfb left a comment

Choose a reason for hiding this comment

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

It also appears that if you provoke a 90 degree angle on the labels by rotating the camera, then the labels disappear.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AspenTech Task owned by AspenTech bug Something isn't working map-component Issues related to the map component.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

well layer: wellNameAtTop control is inverted
3 participants