Skip to content

Conversation

@DevTGHa
Copy link
Collaborator

@DevTGHa DevTGHa commented Dec 11, 2025

Control target_max_lum_nits from LedSettingsBaseModel itself

Summary

For OpenVPCal application, this is duplicated process because LedWallSettings's @property setter has a logic to handle this.
However, the colour calibration plugin use LedSettingsBaseModel instead of LedWallSettings to manage data, it'd be very handy to have this model_validator so target_max_lum_nits can get updated based on target_eotf.

I understand this can be controversial, so I'll back out it this is not desired change.

@DevTGHa DevTGHa requested a review from davisadam10 December 11, 2025 17:48
@DevTGHa DevTGHa self-assigned this Dec 11, 2025
@DevTGHa DevTGHa added the enhancement New feature or request label Dec 11, 2025
@DevTGHa DevTGHa force-pushed the control-target-lum-from-led branch from 7ec1bf6 to ca78a47 Compare December 11, 2025 17:50
@DevTGHa DevTGHa force-pushed the control-target-lum-from-led branch from ca78a47 to 0a44de5 Compare December 12, 2025 16:06
Copy link
Collaborator

@davisadam10 davisadam10 left a comment

Choose a reason for hiding this comment

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

So I think this is where we need to do some kind of cleanup/refactor.

We have moved over to use a pydantic but we have it kinda wrapped that.

I think we would be better to do some more work here, so that we have a single class which we all set and get rather than spreading setters and getters throughout the project.

My 2 cents is lets talk in the new year and we can probably figure out how to make this cleaner.

I'd rather put the effort into revisting the pydantic implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants