-
Notifications
You must be signed in to change notification settings - Fork 4
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
PBRT Docker Support #63
Open
tlian7
wants to merge
16
commits into
DavidBrainard:master
Choose a base branch
from
tlian7:master
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 8 commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
b163633
Remove some default values to match Scene3D output
tlian7 bec10ba
Add a LookAt line in the PBRT file, this seems to be missing when we …
tlian7 a308933
Large - maybe messy - commit that 1) Renders a depth map everytime yo…
tlian7 ff0237a
additional mexopts.sh for newer OS X, matlab versions
benjamin-heasly bd05c53
Converted back to Ben's updated RTB. Added ability to run PBRT from V…
tlian7 a23180f
Fixed pathname error for docker. Added default hints to include docke…
tlian7 f288e38
Fixed issues with using Mitsuba instead of PBRT
tlian7 221f096
Merge branch 'master' of https://github.com/DavidBrainard/RenderToolbox3
tlian7 556b87c
Some cleanup to help the pull request.
tlian7 021aeca
Updated SceneUtilsV1 ...
02217a2
allow PBRT Film to merge with adjustments
benjamin-heasly 90960c7
Merge branch 'master' of github.com:RenderToolbox3/RenderToolbox3
benjamin-heasly 69a5a2b
set PBRT film type from hints whether merging or not
benjamin-heasly ce51ff1
allow Collada lookat -> PBRT LookAt
benjamin-heasly 9af9419
Now rendered image in Blender has same aspect as that of the camera
b4ca7cf
Merge branch 'master' of https://github.com/RenderToolbox3/RenderTool…
tlian7 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Empty file.
Empty file.
Empty file.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Empty file.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -65,17 +65,57 @@ | |
output = fullfile(renderings, [sceneBase '.dat']); | ||
|
||
%% Invoke PBRT. | ||
% set the dynamic library search path | ||
[newLibPath, originalLibPath, libPathName] = SetRenderToolboxLibraryPath(); | ||
|
||
% find the PBRT executable | ||
renderCommand = sprintf('%s --outfile %s %s', pbrt.executable, output, sceneCopy); | ||
fprintf('%s\n', renderCommand); | ||
[status, result] = RunCommand(renderCommand, hints); | ||
if(hints.dockerFlag == 1) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice. This is cool too. May I propose 3 changes? I think these are just fussy organizational things.
If you're on board with these changes, you could edit RenderToolbox3ConfigurationTemplate to add expected fields to the |
||
% We assume docker is installed on this system and we execute the | ||
% function in a docker container | ||
s = system('which docker'); | ||
if s | ||
warning('Docker not found! \n (OSX) Are you sure you''re running MATLAB in a Docker Quickstart Terminal? '); | ||
% TODO: add in option to run on local if docker is not found | ||
else | ||
% Initialize the docker container | ||
dHub = 'vistalab/pbrt'; % Docker container at dockerhub | ||
fprintf('Checking for most recent docker container\n'); | ||
system(sprintf('docker pull %s',dHub)); | ||
|
||
% Start the docker container that runs pbrt | ||
dCommand = 'pbrt'; % Command run in the dockers | ||
[~,n,e] = fileparts(sceneCopy); % Get name of pbrt input file | ||
[~,outstem,outext] = fileparts(output); % Get name of output file | ||
|
||
% We need this line because RTB wants to place the output in | ||
% renderings and not just the recipe folder | ||
outputFile = fullfile('renderings','PBRT',[outstem outext]); | ||
|
||
% rm = clears the container when it is finished running | ||
% -t = terminal to grab tty output | ||
% -i = interactive (not sure it's needed) | ||
cmd = sprintf('docker run -t -i --rm -v %s:/data %s %s /data/%s --outfile /data/%s',copyDir,dHub,dCommand,[n,e],outputFile); | ||
|
||
% restore the library search path | ||
setenv(libPathName, originalLibPath); | ||
% Execute the docker call | ||
[status,result] = system(cmd); | ||
if status, error('Docker execution failure %s\n',result); | ||
else disp('Docker appears to have run succesfully') | ||
end | ||
% disp(r); | ||
|
||
% Tell the user where the result iss | ||
fprintf('Wrote: %s\n',outputFile); | ||
end | ||
else | ||
% Use local PBRT | ||
% set the dynamic library search path | ||
[newLibPath, originalLibPath, libPathName] = SetRenderToolboxLibraryPath(); | ||
|
||
% find the PBRT executable | ||
renderCommand = sprintf('%s --outfile %s %s', pbrt.executable, output, sceneCopy); | ||
fprintf('%s\n', renderCommand); | ||
[status, result] = RunCommand(renderCommand, hints); | ||
|
||
% restore the library search path | ||
setenv(libPathName, originalLibPath); | ||
end | ||
%% Show a warning or figure? | ||
if status ~= 0 | ||
warning(result) | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
Nice. I like what you did here. I think copying files to the resource folder could be useful in other cases as well. Would you be interested in making this a separate behavior, independent of the renderer or Docker?
We could define a separate hint like
copyResources
. Then it could beinstead of
A downside would be that docker users would have to remember to set the value of this new flag. But I think it would be worth it to gain a nice, modular behavior.