chunky-dev / chunky-dev/chunky

[discussion] Refactoring suggestion

Open
#612 6 comments 1 reaction 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.