chunky-dev / chunky-dev/chunky
[discussion] Refactoring suggestion
- Dominant language
- Java
- Stars
- 738
- Forks
- 78
- Avg merge
- 10h 5m
- Merged PRs (30d)
- 3
Description
During the past few days I have been going through the code as I'd like to contribute (more than just one-line-change PRs :smile: ), especially to the renderer. I have noticed a few things and I'd like to open a discussion about them.
My main concern is that the `Ray` class does too much, it contains input (origin and direction), output (distance, materials, …) and temporary (t, tNext, …) values. This leads to functions that operate through side-effects modifying the `Ray` passed in input even though these could be pure (for instance, the [`TexturedBlockModel.getIntersectionColor(Ray ray)`](https://github.com/llbit/chunky/blob/573424f349117937bb5112ddb1418395286b00a5/chunky/src/java/se/llbit/chunky/model/TexturedBlockModel.java#L132) function doesn't actually return a color but has the side-effect of setting the ray's color, and uv fields to the result). This makes the code harder to read as what part of the `Ray` is modified by a function call must be kept in mind, an operation might overwrite a previous result, etc. making the computation harder to follow and bugs harder to solve.
My suggestion would be to extract all the output members to other classes and make `Ray` only contain its origin, direction and the medium it traverses. Intersecting the scene would give a `Hit` or `HitRecord` result that contains information specific to a hit that can be used for path construction and shading (distance, normal, materials, …). The temporary values such as `t` and `tNext` should be local to the functions that use them.
I would like to work on this but it is a big change so I wanted to get some opinions, and some feedback in case I made an oversight as the codebase is still fairly new to me.
Contributor guide
Research direction
Start with the Ray class and TexturedBlockModel.java, especially getIntersectionColor(Ray ray), to understand the renderer state and side effects described in the discussion. The issue does not define a concrete completion condition; feedback and agreement on the proposed Hit or HitRecord design would be needed before implementation can be scoped.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- java
- Domain
- computer-graphics
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 20/100