[V2] Add operation to save image snapshots to local disk. #599
Conversation
The period and path prefix and suffix can be set, and the operation enabled and disabled at will (this is particularly useful in combination with PR WPIRoboticsProjects#572).
|
|
||
| if (!inputSocket.getValue().isPresent()) { | ||
| throw new IllegalArgumentException("Input image must not be empty"); | ||
| } |
There was a problem hiding this comment.
This isn't necessary.
Step does this already.
Current coverage is 58.78%
@@ master #599 diff @@
==========================================
Files 191 194 +3
Lines 5963 6041 +78
Methods 0 0
Messages 0 0
Branches 553 556 +3
==========================================
+ Hits 3508 3551 +43
- Misses 2293 2330 +37
+ Partials 162 160 -2
|
AustinShalit
force-pushed
the
image-snapshots
branch
from
6843281 to
7648094
Compare
AustinShalit
force-pushed
the
image-snapshots
branch
from
7648094 to
aae316c
Compare
| } | ||
|
|
||
| final Handler fileHandler = new FileHandler("%h/GRIP.log");//Log to the file "GRIPlogger.log" | ||
| final Handler fileHandler = new FileHandler("%t/GRIP.log");//Log to the file "GRIPlogger.log" |
There was a problem hiding this comment.
Why are you changing the log file directory?
There was a problem hiding this comment.
Never actually finished what I was doing... Going to move the log file into the GRIP directory.
| } | ||
|
|
||
| // don't save new image until period expires | ||
| if (stopwatch.elapsed(TimeUnit.NANOSECONDS) < periodSocket.getValue().get().doubleValue()*1000000000L) { |
There was a problem hiding this comment.
Why are you using nanoseconds? There's no real point if the file name has only millisecond precision.
AustinShalit
force-pushed
the
image-snapshots
branch
from
c3fbe67 to
c73500b
Compare
AustinShalit
force-pushed
the
image-snapshots
branch
from
c73500b to
b352d9a
Compare
AustinShalit
force-pushed
the
image-snapshots
branch
from
98aa692 to
cfbe494
Compare
|
@SamCarlberg @JLLeitschuh I added the filetype selector. The only issue is that the quality slider does not do anything when png is selected (this is intentional). Do we care that it will not do anything? |
| logger.log(Level.WARNING, ex.getMessage(), ex); | ||
| } | ||
| }; | ||
| new Thread(runnable).start(); |
There was a problem hiding this comment.
Make this a daemon thread otherwise it won't die when the application tries to shut down.
This is especially important if the file writing hangs.
AustinShalit
force-pushed
the
image-snapshots
branch
from
586e9cf to
3dbb57c
Compare
|
This PR is ready for review/merge |
|
LGTM |
| activeSocket = operation.getInputSockets().stream().filter( | ||
| o -> o.getSocketHint().getIdentifier().equals("Active") | ||
| && o.getSocketHint().getType().equals(Boolean.class)).findFirst().get(); | ||
| } |
There was a problem hiding this comment.
Do you really need dependency injection for this?? There is a MockInputSocketFactory and a MockOutputSocketFactory you can use.
Also, why are you using a real file manager in a test? I really don't want tests writing to the file system.
There was a problem hiding this comment.
I don't believe I am using a real FileManager. I will remove the injection
| IMAGE_DIRECTORY.mkdirs(); // If the user deletes the directory | ||
| Files.write(image, file); | ||
| } catch (IOException ex) { | ||
| logger.log(Level.WARNING, ex.getMessage(), ex); |
There was a problem hiding this comment.
Why does this not pass the exception up to the caller?
There was a problem hiding this comment.
At this point it is in a different thread. How could I handle that?
There was a problem hiding this comment.
Oh, I suppose this doesn't make sense.
5 participants
Closes #573
This PR is a continuation of #573