Conversation
|
Did you use LDView's OBI code for the high-contrast work? (I'm guessing not, since it requires modified parts.) The OBI code was added years ago, but never became mainstream due to the requirement of having updated parts. |
I did not. The high-contrast behaviour simply extends the primitives in TCStudLogoPrimitive StudLogoPrimitives and adds the ability to manipulate the stud cylinder and edge line colour both manually and automatically. The resulting behaviour adds an 'automate edge line color' checkbox to the Geometry tab and two new options to the Primitives tab's 'use stud logo geometry' combo control: HighContrast =6 High Contrast Plain
HighContrastSingleWire =7 High Contrast Single WireCheers, |
|
@pbartfai Do we want to update this PR to support macOS and Qt before releasing 4.7 or after? (Qt for you, macOS for me.) |
Let's release 4.7 first. |
|
Great. I'll do the macOS one. Do you know what the best way is to handle
this? I think it's probably easiest to just merge Trevor's Windows-only PR
to our master and then create new PRs for Qt and macOS.
Note: I have not yet reviewed Trevor's PR, but I will do that. Can you
include a Qt screenshot in your PR description? I'll do one for macOS once
I have it implemented.
…--Travis
On Tue, Mar 17, 2026 at 4:08 PM Peter Bartfai ***@***.***> wrote:
*pbartfai* left a comment (tcobbs/ldview#101)
<#101 (comment)>
@pbartfai <https://github.com/pbartfai> Do we want to update this PR to
support macOS and Qt before releasing 4.7 or after? (Qt for you, macOS for
me.)
Let's release 4.7 first.
@tcobbs <https://github.com/tcobbs> 4.7 has been released
I've prepared the Qt UI...
—
Reply to this email directly, view it on GitHub
<#101 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AE4BDKKQDDTTMILLWC6KEQL4RHEHBAVCNFSM6AAAAACKWOSG3CVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DANZYGI3TMMBUGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
@trevorsandy Sorry for the delay. I am finally ready to review this and hopefully merge, but I have hit some possible problems. I'm guessing that this PR includes all of the local changes that you have made to LDView for LPub3d, because there is a bunch of stuff in here that is totally unrelated to the stud logo settings the that PR title makes this seem to be. (You do state in the description that this includes high-contrast studs and edge lines, and maybe that's all that's here in addition to the stud logos.) While I don't have a problem in principle of incorporating all of these changes, I would greatly prefer if they didn't all show up in one single PR. (Having said that, I am not saying that separate PRs is an absolute requirement.) How hard would it be to narrow down this PR to just the stud logo changes, and then create one or two other PRs for the other changes? I promise that I will deal with them in a timely manner. Finally, can you confirm that this PR only contains the following:
|
|
After starting to review the PR, I realized that it has two (maybe three)
unrelated features. I asked Trevor if he would be willing to split it into
multiple PRs. I'll see what he says.
…--Travis
On Tue, Mar 17, 2026 at 7:19 PM Travis Cobbs ***@***.***> wrote:
Great. I'll do the macOS one. Do you know what the best way is to handle
this? I think it's probably easiest to just merge Trevor's Windows-only PR
to our master and then create new PRs for Qt and macOS.
Note: I have not yet reviewed Trevor's PR, but I will do that. Can you
include a Qt screenshot in your PR description? I'll do one for macOS once
I have it implemented.
--Travis
On Tue, Mar 17, 2026 at 4:08 PM Peter Bartfai ***@***.***>
wrote:
> *pbartfai* left a comment (tcobbs/ldview#101)
> <#101 (comment)>
>
> @pbartfai <https://github.com/pbartfai> Do we want to update this PR to
> support macOS and Qt before releasing 4.7 or after? (Qt for you, macOS for
> me.)
>
> Let's release 4.7 first.
>
> @tcobbs <https://github.com/tcobbs> 4.7 has been released
> I've prepared the Qt UI...
>
> —
> Reply to this email directly, view it on GitHub
> <#101 (comment)>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AE4BDKKQDDTTMILLWC6KEQL4RHEHBAVCNFSM6AAAAACKWOSG3CVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DANZYGI3TMMBUGY>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
|
The easiest would be merging this PR first then update Mac and Qt code. |
|
Hi Travis, I can confirm that this PR only contains the following: Stud logo options I was deliberate to remove content that only implicated LPub3D such as the ability to suppress high-contrast colour management for 'fade previous steps' and 'highlight current step' behaviour.
There is no code in this PR that is not necessary to implement the behaviour outlined above. If you remain convinced otherwise, I will be pleased to clarify any suspicious code you care to specifically reference.
There are 7 (essentially 5 with 2 build fixes) commits in this PR. The first 2 commits delineate the stud logo preference behaviour:
...and 3-5 address the manually selected or automatically applied, stud cylinder and edge color management :
As you can see, with the exception of f8dcb51 which transitions naming for better maintainability, these commits are broken down into control logic and Windows UI implementation. This shows that the optimal PR breakdown would create 2 PRs:
If this breakdown is acceptable to you, I will be pleased to close this PR, and create 2 new ones respectively ? Cheers, |
FWIW, stud logo and edge line colour management was added to LeoCAD, in quite the same manner as this PR. Leo added his change commits to the PR branch until he was satisfied that the content reflected what he wanted - at this point he merged the PR to master. This is also how I manage large updates so as not to implicate the best-so-far state of master until I am pleased with what is being added. Cheers, |
Thanks for your replies. I can accept leaving this PR alone. It may take me a few days to review all the changes. |
tcobbs
left a comment
There was a problem hiding this comment.
@trevorsandy I'm still reviewing, but I wanted to submit the comments on the code that I have so far reviewed.
LDLoader/LDLModel.cpp
Outdated
| if (sm_studCylinderColorEnabled) | ||
| return input; | ||
|
|
||
| const std::string studColor = "4242"; |
There was a problem hiding this comment.
Given the weird color numbers being used in LDConfig.ldr, how safe is 4242?
Many thanks for sharing these review remarks, they present valuable insight into the architectural and design constraints of LDView that I did not take into account when I implemented this update. If I may, it would be quite a bit more efficient and productive to reflect your changes as commits to the PR branch. Like this, your preferences and changes will be communicated just as well and we could still discus any changes that require clarification while actually evolving the PR towards what you would find acceptable for integration into LDView. Cheers, |
For that we would need permission to push commits to your branch. See https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork |
Travis was added as a collaborator on the entire fork many years ago. I sent you an invitation just now. Cheers, |
|
OK, sounds good. I'll commit to your branch. Because there as so many changes, it's likely going to take a while. |
Co-authored-by: Travis Cobbs <[email protected]>
|
@trevorsandy I have committed some fixes to resolve my comments. It would probably be good if you periodically pull and look at the new commits and try to verify that I'm not breaking things. |
| #define _APS_NEXT_RESOURCE_VALUE 550 | ||
| #define _APS_NEXT_COMMAND_VALUE 40122 | ||
| #define _APS_NEXT_CONTROL_VALUE 1325 | ||
| #define _APS_NEXT_CONTROL_VALUE 1357 |
There was a problem hiding this comment.
This is just here to prevent an accidental merge before I get this far.
I reviewed through 9b7eaf5 and did not see anything suspicious. I would however like to point out your refactor that adds LDPalette to LDPreferences. I wanted to do this and pondered it for many days, but in the end, was unsure of the full implications so I refrained. Your implementation valades my instinct and it is quite a pleasure to see how you provisioned this update! In fact, your move should solve the different palette load times between the snapshot and interactive modelview flows which could, in certain CLI/snapshot use cases, see the palette initialized before the specified colour parameters were loaded. Well done. Cheers, |
* Remove hard-coded (and edited) LDraw library files.
* Load the files from the library on-demand
* Algorithmically produce color 4242 versions when needed
* Use istringstream to load generated files instead of saving to temp
|
@trevorsandy This is a big commit to get rid of the hard-coded stud files and also load the generated strings using a string stream instead of writing files to temp. I tried to test the various code paths, but I may have messed something up. Please give it a try with the various different style options. |
On a fresh build of the STUD_LOGO branch using StudStyles.ldr (see Details above), the overall behaviour seems as expected:
Here are the exceptions I observed:
Cheers, |
Just so I'm on the same page. If I enable stud style and set stud style to 7, are the sides of stud.dat supposed to be black? |
|
@trevorsandy The '7 High Contrast Single Wire' problem you reported should now be fixed. The problem was the presence of 4-4cylc.dat in the library's stud-logo.dat file. |
|
@trevorsandy Question: are transparent studs supposed to be getting the black sides? That looks very strange to me, but I can see how preventing it might be hard. |
Yes - however to be precise, the color should be that which is set as the stud cylinder color. The default stud cylinder color is 27,42,52,255 which is not quite black. Cheers, |
Yes - I pondered making opaque cylinders on trans-clear studs optional using a meta command (in LPub3D) but at the moment, trans-clear studs are opaque. Here is an example: Cheers, |
| { | ||
| } |
| { | ||
| } |
There was a problem hiding this comment.
@trevorsandy Is there a reason this is empty?
Yes - there is nothing to do - unlike the void LDViewPreferences::doNewPrefSet(void) function just before.
Cheers,
|
On a fresh build of the STUD_LOGO branch (at ff8d8ea) using StudStyles.ldr (see Details above), the overall behaviour seems as expected:
However, these exceptions remain:
This behaviour unsettles the UX. I propose that for the GUI, 'Use stud style geometry' and 'Low quality studs (Faster)' should be an either/or selection, with the unselected option disabled to properly indicate the behaviour will not function when the alternate option is selected. It is a bit odd that, on reviewing the code, I could not find where the stud style behaviour is ignored when low quality studs is enabled. Cheers, |


Enable the ability to select stud logo preference using the following stud logo geometry:
0 Plain
1 Single Wire
2 Double Wire
3 Raised Flat
4 Raised Rounded
5 Subtle Rounded
Only the Windows GUI is implemented; however from other platforms (i.e. macOS/Linux), you can set the the following options using [Sessions] in your .ini file.
This feature is a subset of the comprehensive behaviour which also includes high-contrast studs and edge lines.
With the addition of high-contrast studs and automatic edge line colors, the stud style ini file options have changed to:
And stud geometry options are extended to:
0 Plain
1 Single Wire
2 Double Wire
3 Raised Flat
4 Raised Rounded
5 Subtle Rounded
6 High Contrast Plain
7 High Contrast Single Wire
Cheers,