This repository has been archived by the owner on Sep 20, 2024. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Maya looks: support for native Redshift texture format #2971
Maya looks: support for native Redshift texture format #2971
Changes from 11 commits
3770eb6
72b4522
e65a1ea
2406f78
be840d3
d4b8d47
abc299e
640415a
0bcf353
7bbf381
078775e
6ac5cd4
c43ec04
c36552e
92c1ac7
74f2c78
7b1346e
ae8ecfe
3c3be40
0bebd06
0100ea5
2102e4f
406ac82
710ed3a
68caaa7
a74e8c2
29b69bc
e78314c
00d877e
2933e40
b054175
fd3125d
41e7ac7
35be816
cfb9093
eb99944
77c15e0
8f8845e
e5fdd9e
ab14895
c471bbe
5f4d06b
9098822
9b0cc4d
c35f1cf
099dfba
8995dcd
787c797
3cd7fd5
ac0b931
0179c63
d28c7e1
9ec42cb
cc62035
09bbb30
ad8177c
7413ca6
590538e
035281f
18435fa
3728ce0
c9c2635
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Note: I think we should validate this environment during validation, only when is needed if it's possible to know.
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.
Ah, I see! Thank you, I'll take a look about moving that
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.
That's something I don't know. Maybe it's not needed or impossible to validate before this plugin, can't tell.
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.
I see, thank you!
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.
This is more complex as this is set only if studio is using and has configured Redshift. So I think the logic should be - add option
make redshift texture
on the same place as we have with make tx (look instance) and then probably have validator that will run on look family and will validate this env var.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.
Few notes that are connected to each other:
run_subprocess
fromopenpype.lib
which would do what you do here with subprocessescape_space
will not be needed in that caseAt this stage are these notes just recommendation because I don't know how it will be used.
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.
Thank you! This is helpful, makes me think whether we should consider refactoring the
maketx
function as well to simplify a little bit.Will hop on to something else in the meantime. I appreciate your time!
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.
Yes thank you I know :D
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.
Not a huge issue - but this does feel off here since unlike the rest of the methods here this Redshift binary isn't actually vendorized? I do like that there's unison in the function itself however - nonetheless it's not perfect.
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.
Thank you @BigRoy, you bring up a good point. I wonder where it would be good to place the function then @antirotor @iLLiCiTiT would love your take on where I should be placing this
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.
I would probably define separate ENV var for the tool that should be defined in the tool itself to use it.
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.
is it necessary? why can't we use the mandatory
{REDSHIFT_COREDATAPATH}/bin/redshiftTextureProcessor.exe
(and the appropriate lnx and mac variant)