-
Notifications
You must be signed in to change notification settings - Fork 83
Fix #4195: Preserve transforms when switching PBR materials #4725
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
base: develop
Are you sure you want to change the base?
Fix #4195: Preserve transforms when switching PBR materials #4725
Conversation
pre-commit fails due to spaces. I might be missing something, but the variant from previous PR looked better, sans lack of convertTextureTransformToPBR. |
In the previous PR, applying PBR had an issue: texture values were only preserved in the preview. When clicking OK, the values reset to defaults (though changing them after applying PBR did preserve them). In addition, PBR-to-PBR conversions didn’t preserve values and subsequent PBRs still picked up texture values. |
I suspect that's the issue with queueApply, basically doing 'queueApply(hit_obj, hit_face, NEW_asset_id, preserved_override)' works fine, but doing 'queueApply(hit_obj, hit_face, OLD_asset_id, preserved_override)' would ignore preserved_override and drop the overrides. If that's indeed the cause, the solution is to compare current material id and if it didn't change then not send anything at 'OK', nothing changed after all. |
You were right: the OK issue came from calling queueApply with the same material ID, which cleared overrides. |
I think it did work in your closed PR with a 'drag&drop material over previous material' case. |
ffb5637
to
cc84959
Compare
@Aqil-Ahmad, @akleshchev has done some investigation and determined that we are blocked from completing this feature because of a server-side issue. I'd like to propose the following:
|
I was wrong and it was a viewer issue, fixed. Please update from develop. I tested code from this PR (removed everythign from llpanelface.cpp and from llgltfmateriallist.cpp sans convertTextureTransformToPBR ) and the only issue appears to be a missed existing_override check (convertTextureTransformToPBR fires in case of a pbr-pbr transition), otherwise everything works as expected. |
cc84959
to
c505d35
Compare
c505d35
to
1446d59
Compare
sApplyQueue.push_back({ obj->getID(), side, asset_id, material }); | ||
} | ||
|
||
if (sUpdates.size() >= MAX_TASK_UPDATES) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you removing this part?
A 'send' happening here is a questianable practice, but removal doesn't seem relevant to transform preservation. Otherwise changes seem solid if a bit too many repeats of the same code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did some additional testing and everything seems to be in order, will squash tommorow then remove the 'material list not sending' part since that seems to be unintended result from a merge conflict.
Description
Fixes texture transforms being reset when switching from Blinn-Phong to PBR materials and between PBR. Previously, custom scale, offset, and rotation settings would be lost, making it tedious to switch between PBR materials.
Related Issues
Issue Link: #4195
Checklist
Please ensure the following before requesting review:
Additional Notes
scndLife_vid.mp4