Vertical elevation leg - #308
Conversation
…ul for when going accross passages and for some avens and pitched. This is not supported by survex!
| // which is the actual height change. This correctly preserves the station's altitude | ||
| // without inflating it the way forcing inclination to ±90° would. | ||
| float dz = leg.getDistance() | ||
| * (float) Math.sin(Math.toRadians(leg.getInclination())); |
There was a problem hiding this comment.
Yay, maths 😄
This looks overly complicated. I haven't tested it, but to get the effect you're looking for, can't we simplify this by adjusting the azimuth to 90 degrees (i.e. coming straight out of the "page") and letting the existing machinery handle it?
There was a problem hiding this comment.
It is the same maths as left and right use, well sort of.
I think the whole maths behind the sketches can be improved and rationalised, however that is probably a total rewrite, or at least a fundamental changes. I'm not sure my coding ability is up to that, as is something that is fundamental to the working of ST. AI maths seems to be quiet poor as well.
I did look at using the existing machinery, but that would make the overhead higher.
There was a problem hiding this comment.
This bit of code in your PR violates a few principles of good software engineering.
One, Don't Repeat Yourself (DRY). For any particular piece of logic we ideally want to centralise it one place in the codebase. If it's in more than one place then there's multiple places bugs can hide, and multiple places to change something if we think of a better way of doing it. If you have lots of bits of code doing similar things then you end up with a big mess. We already have one bit of logic where we switch from polar coordinates to cartesian, so we don't want it calculated here as well.
Another is that code in one function should be at the same level of abstraction. You're mixing low-level maths here (trig) with high level concepts (mapping the centreline to the extended elevation). It's much easier to read, reason about and test if it sticks to one representation.
There's also an early return in there, which is generally considered a Bad Idea because it's easy to miss that one branch has a different pathway through the code.
Performance might be better to do the maths in place, but this bit of code will only be called once per entire draw cycle, so it's really not an issue here.
I think the whole maths behind the sketches can be improved and rationalised
What did you have in mind? I think it's actually all mostly pretty elegant (this is of course part of the projection / data code, not the sketching!)
It could be made more performant (e.g. rewrite it all with linear algebra or use a graphics engine that uses the graphics processor) but there are tradeoffs and this seems to work pretty well.
There was a problem hiding this comment.
I think the whole maths behind the sketches can be improved and rationalised
What did you have in mind? I think it's actually all mostly pretty elegant (this is of course part of the projection / data code, not the sketching!)
Yes I do mean the projection /data code. I haven't dived deeply enough into and as you can see from above this part of coding is right on my limit. At times if feels like unnecessary calculation are made, however, sometime more elegant maths is not elegant programming and you are a far better coder than me.
As you say it works very well so it is unlikely to ever get close to the top of my list.
There was a problem hiding this comment.
I think 4ec7df2 is better, but I'm at the edge of my understanding
| space.addLeg(leg, new Line<>(start, end)); | ||
| if (leg.hasDestination()) { | ||
| // delta=0: subsequent legs are not rotated by this vertical leg's azimuth | ||
| update(space, leg.getDestination(), end, 0); |
There was a problem hiding this comment.
Don't we potentially lose any existing L/R direction inherited from the incoming leg?
| RIGHT, | ||
| VERTICAL; | ||
|
|
||
| public Direction opposite() { |
There was a problem hiding this comment.
This method is no longer used; we can delete
| <string name="menu_elevation">Elevation</string> | ||
| <string name="menu_draw_left">Draw Left</string> | ||
| <string name="menu_draw_right">Draw Right</string> | ||
| <string name="menu_draw_vertical">Draw Vertical</string> |
There was a problem hiding this comment.
Maybe, it does need something better, place holder for now!
| Direction grandparentDirection = grandparent.getExtendedElevationDirection(); | ||
| // If the grandparent is also VERTICAL, keep climbing until we find a real direction | ||
| // or exhaust the chain — but for simplicity one level is sufficient for typical caves. | ||
| return (grandparentDirection == Direction.VERTICAL) ? Direction.RIGHT : grandparentDirection; |
There was a problem hiding this comment.
Better to write this recursively here... something like
private static Direction resolveInheritedDirection(Survey survey, Station station) {
Direction activeDirection = station.getExtendedElevationDirection();
if (activeDirection != Direction.VERTICAL) {
return activeDirection;
}
// Walk up one level to find the grandparent station's direction
Leg referringLeg = survey.getReferringLeg(station);
if (referringLeg == null) {
return Direction.RIGHT; // origin station — nothing above it
}
Station parentStation = survey.getOriginatingStation(referringLeg);
return resolveInheritedDirection(survey, parentStation);
}
| if (projectionType == Projection2D.EXTENDED_ELEVATION | ||
| && leg.hasDestination() | ||
| && leg.getDestination().getExtendedElevationDirection() == Direction.VERTICAL) { | ||
| drawDashedLine(canvas, start, end, verticalLegDashIntervalPx, paint); |
There was a problem hiding this comment.
Dashed line is used for legs "not in plane" so possibly confusing. Maybe we don't need a visual indicator here.
There was a problem hiding this comment.
That is a good point, essentially a vertical plot is the leg coming out of or going into the page, so it should probably use the dashed of 'not in plane.' Does that make sense?
There was a problem hiding this comment.
Used the same as out of plane in elevation 7fa9bb6
Added missing file
2 participants
For some situations it is better to ignore the left or right part of the extended elevation and only use the height change. As an example, this happens when a leg goes across a passage.
Added the ability to make a leg only use the vertical height change.
This is supported by Therion, but it appears not by Survex. However, the extend export for Survex is currently wrong and does not work.
In commit 7f6b133 I thought that the forced to vertical legs should be shown some how, I've tried making them dashed. Not sure this is the right solution but I do feel they should be differentiated somehow