Scene view #8
Scene view #8
Conversation
1 similar comment
Codecov Report
@@ Coverage Diff @@
## dev #8 +/- ##
=============================================
- Coverage 84.17% 72.93% -11.24%
- Complexity 147 176 +29
=============================================
Files 23 31 +8
Lines 455 606 +151
Branches 38 44 +6
=============================================
+ Hits 383 442 +59
- Misses 69 161 +92
Partials 3 3
Continue to review full report at Codecov.
|
|
|
||
| public class Input { | ||
|
|
||
| private boolean forwardKeyDown = false; |
There was a problem hiding this comment.
Assignment is redundant. Is it made for clarity?
There was a problem hiding this comment.
Yes, to make it clear that my default all keys are not down.
src/main/java/labs/introtoprogramming/lab5/scene/ControllableCamera.java
Outdated
Show resolved
Hide resolved
|
|
||
| public abstract void addSceneObject(SceneObject obj); | ||
|
|
||
| public void addSceneObjects(SceneObject... obj) { |
There was a problem hiding this comment.
Maybe make this function abstract and make abstract class as interface?
There was a problem hiding this comment.
This method implementation is likely to stay the same for all implementations. So doesn't make a lot of scene to make it abstract.
| return false; | ||
| } | ||
|
|
||
| public void setInput(Input input) { |
There was a problem hiding this comment.
Most of object are static and this functionality is redundant for them. Maybe is it more reasonable to get data from input and than apply it to object via transform?
| private Input input; | ||
|
|
||
| public SceneRendererWindow(Scene scene) { | ||
| this(new BasicRaytracingRender(scene.getCamera().orElseThrow(NoCameraException::new).raster()), scene); |
There was a problem hiding this comment.
Maybe is it better to refactor SceneRender to only accept scene, if raster can be obtain from camera now? Or is it out of place for this PR?
There was a problem hiding this comment.
Multiple constructors increase flexibility. There should be a way to set custom scene render.
There was a problem hiding this comment.
I am talking about basicRaytracingRender. Maybe is it better to add constructor that only accept scene as argument and take raster from camera?
No description provided.