CODING_GUIDELINES.md
This document describes how we write, test, and review code in Genesis. It is aimed at all contributors. It is long on purpose: most review friction on this project comes from small, well-defined conventions that are easy to follow once you know them, and expensive to re-explain on every pull request. Reading this once will save you (and your reviewer) many review rounds.
Two things to keep in mind throughout:
genesis/engine/solvers/rigid/ and imitate what you find there. The historical rationale for the naming scheme is documented in PR #1053.Every addition is a cost before it is a benefit: a new algorithm adds maintenance burden, and modifying an existing one risks new bugs. A pull request is merged only when its benefit to end users outweighs that cost - "it works" is not a benefit, since working on its own brings nothing concrete. Benefit is measured on three axes, and a contribution that improves none of them is rejected:
tests/benchmarks/ - above +5% on at least one. Any regression counts against the contribution, weighed case by case against which benchmark it hits. If a real gain is invisible to the current benchmarks, extend the suite so it shows up rather than asserting it in prose.noslip_iterations), not incidental knobs (noslip_tolerance). A new option whose default works out of the box everywhere is not a real parameter, but then the contribution is judged as if that default were hardcoded: an algorithm reachable only by flipping a non-default flag brings zero benefit on its own. If the flag is instead meant to be tuned, it counts as a real parameter, and its cost must be bought back by the speed or robustness it unlocks.Consistent naming is one of the most heavily enforced aspects of Genesis reviews. The scheme below applies everywhere, including throwaway debug and instrumentation code: a cryptic one-off local like wt (for is_watertight) will be flagged even in a script you plan to delete.
link_idx, never LinkIdx.joint_ not jnt_, link_ not ln_. The only tolerated shorthands are idx for index and n for num.entity, a link, or a geom - never coin a new term (body, object, piece) when one of these fits. This is also a correctness cue, not just naming: the rigid-body unit is the link, so a per-object quantity groups geoms per link (iterate entity.links then link.geoms), never per entity - assuming entity for a multi-link entity is a bug.is_ or has_ prefix, present tense preferred: is_hibernated (not hibernated), is_fixed, is_convex, has_multi_island_structure. was_ is acceptable only for a genuine past-tense flag (prefer is_cached_loaded over was_cached). A bare name (hibernated, placed) or a did_ prefix (did_fuse) is not valid.has_any_rigid_coupling() method) should be properties instead.all_ prefix or suffix on containers; name the content specifically: joints_xanchor, links_inertia_i, geoms_pos, entities_quat, verts_idx.mass_mat_env = mass_mat_all; they add a second name for the same object and nothing else.ipc_*.py).s on the first word only, even when it reads oddly: links_dof_start, not dof_start_links and not link_dof_starts. Alternatively, use a container-type last word: _pair, _set, _map, _list.geoms_vertices_idx.i_<short code>, where the code is a short type tag, usually one or two letters: i_b (batch/environment), i_d (dof), i_c (contact), i_l (link), i_ga/i_gb (geoms a and b of a pair). Verbose forms like i_hull_top are rejected; make the code slightly longer only when needed to resolve a genuine ambiguity. The rule applies to stored values too: i_pair = ..., not pair_idx = ....j_ prefix: i_d/j_d for two nested dof loops. Numbers are allowed in the code when meaningful: i_d0/i_d1 for two perturbation directions.i_c_ and use it as field[i_c_start + i_c_, i_b]. The absolute range boundaries are i_c_start/i_c_end, and counts are n_* (n_con, n_kept). A bare k used as b_start + k violates both rules at once._idx suffix is reserved for persistent tensors and fields (contact_sort_idx, dofs_idx). Never use _idx for a local loop counter or a local holding an index value.i_b (batch/env), i_c (contact), i_d (dof), i_e (entity), i_g/i_ga/i_gb (geom), i_l/i_la/i_lb (link), i_v (vertex), i_f (face), i_con (constraint), i_pair, i_p/i_q (current/previous item in a sort, with matching _p/_q attribute suffixes), i_pc (physical contact slot), i_cb (contact within a bucket). A capitalized I_l denotes the maybe-batched index tuple [i_l, i_b] if batched else i_l.@qd.func that processes a single environment (first argument i_b), called from a kernel's batch loop, is suffixed _batch: func_solve_mass_batch, func_noslip_batch.tau and qddot, not Greek letters; * not a middle dot; x not a multiplication sign; <=, >=, !=, ~= not their Unicode forms; - or -- not an em-dash; plain # --- separators, never box-drawing characters. After bulk edits, LC_ALL=C grep '[^[:print:][:space:]]' <files> should come back empty.Cloth, not ClothMaterial.isinstance result; write isinstance(...) directly in the if._adaptive_params(verts, faces, aggressiveness=7), not a bare 7. Positional is fine for self-named variables. This holds in rigid kernel and func calls too (see section 9): a bare True, True, False tail is unreadable, so anonymous constants and everything after them go by keyword.The default is no comment. A comment earns its place only when a reader looking at the code would reasonably ask "why does this work?" or "why is this here?" and could not answer from the names and control flow alone: a subtle invariant, a workaround for a specific upstream bug, a non-obvious ordering constraint, a hidden assumption.
ellipticCostDif in engine_solver.c" is not.-, not double --; single-line comments do not end with a period; no RST double backticks in # comments (they only render in docstrings).print statements are prohibited; convert them to gs.logger.debug traces.@qd.kernel/@qd.func bodies alike, so that IDE preview popups show one sharp sentence.# comments at the top of the body.None and is resolved internally, document it as: "If None, resolved based on <condition>: <value1> for <case1>, <value2> for <case2>. Defaults to None." Do not write "resolved at build time" or similar implementation-timing details; the reader cares about the conditions and outcomes, not when resolution happens.Genesis strongly resists the accumulation of small wrappers. Helpers accrete, get reused where their assumptions do not hold, and force reviewers to jump around to confirm one-liners.
genesis.utils and serve any caller. Anything beyond that is suspect._foo). They are almost always the wrong tool. Prefer, in order: inlining at the call site (duplicating a one-to-three-line check is not a sin), a module-level free function when the logic has no self dependency, or an attribute computed once at build time when the value is stable and reused. Extraction pays for itself only at three or more call sites with genuinely non-trivial logic._post_process?") stays inline at its single use site - no private class attribute, no classmethod, no staticmethod for it.isinstance guard, an is None check, or a try/except that the canonical version omits, treat that as a red flag: prove it is needed by removing it and running the relevant test, or drop it. Do this before writing a comment to justify the guard.self.abd_data_by_link.clear(), not self.abd_data_by_link = {}.genesis/__init__.py, and the quadrants dataclass dtypes live in genesis/utils/array_class.py..long(), .to(torch.int64), ...). For advanced indexing, use indices_to_mask from genesis.utils.misc; for a torch.gather index, let a cumsum over a boolean mask produce int64 naturally.detach().cpu().numpy(). Use tensor_to_array from genesis.utils.misc, which handles the GPU-to-CPU transfer correctly. Never call np.asarray on a torch tensor. It takes a dtype argument (tensor_to_array(x, dtype=np.float64)) - use that rather than chaining .astype, and do the conversion once inside the function that needs the dtype, not at each call site (callers pass native entity.get_verts() / geom.get_trimesh().faces).float(), int(), .astype(...), np.asarray(...), or a dtype= unless strictly necessary. A numpy scalar indexes arrays, compares, does arithmetic, and satisfies assert x < tol fine; wrapping it for a local, return, or assert is noise, and re-stating a dtype the data already has (e.g. trimesh.vertices is float64) is the same. Cast only where an external interface forces it (igl needs float64 verts / int64 faces; kernel args need native float/int), and never re-cast what a later step already casts (drop per-chunk casts before a final .astype).gu.* math helpers, and setters speak torch natively: keep them pure torch (torch.as_tensor(geom.init_verts, dtype=gs.tc_float), torch.zeros(..., dtype=gs.tc_float), torch.linalg.norm), with no tensor_to_array round-trips in between. tensor_to_array is for crossing into numpy-only consumers (np.testing, matplotlib, trimesh/igl). Note gu.transform_quat_by_quat and friends reject mixed torch/numpy inputs.dtype=gs.tc_float / gs.tc_int, never dtype=other_tensor.dtype.gu.xyz_to_quat(euler) (radians, torch-capable), not gu.euler_to_quat (numpy-only) and not hand-rolled quaternion literals.gu.transform_by_quat(v, quat), not by materializing the matrix (gu.quat_to_R(quat) @ v). Reserve quat_to_R for when the matrix itself is needed (e.g. extracting a frame axis as a column).np.asarray in general is reserved for public API entry points where the data source is uncontrolled. It should never appear on internal code paths.numpy.float64, ...) as kernel arguments; cast to native Python float()/int(). Numpy scalars break the quadrants fast-cache, which holds weak references.flatten(), reshape((-1,)), or ravel() where a squash is meant, and prefer [..., 0, :] over squash(axis=-2).getattr/hasattr are prohibited on our own classes; initialize attributes to None and discriminate with isinstance. The one narrow exception is objects from an external library, and only when no isinstance check against a public, stable type exists. Decision order: public-type isinstance first, getattr(obj, "name", default) second, try/except AttributeError as a last resort.entity.links, link.geoms, geom.init_verts, link.get_pos(), and so on. Drop to low-level solver fields only when efficiency, conciseness, or maintainability specifically demands it.qd_to_numpy / qd_to_torch from genesis.utils.misc for bulk GPU-CPU transfers. Calling the quadrants .to_numpy() / .to_torch() methods directly is forbidden, and so is item-accessing a qd field (field[i]).transpose=True so the batch dimension comes first ([B, n, dim]), matching the public getter convention.copy= at all when the returned value is fresh arithmetic; let the function zero-copy or copy as needed. Reserve copy=True for the case where a raw field view would otherwise escape and could be mutated later. Note that copy=False fails on CUDA: GPU data always requires a copy to reach numpy.qd_to_numpy returns a live view into the field buffer. If you store the result across simulation steps, .copy() it first (or reduce it to Python scalars immediately) - otherwise a later "delta" between stored and current values is identically zero, because both names alias the same memory.envs_idx=env_idx if batched else None).@qd.kernel - no @qd.data_oriented classes. Use V_ANNOTATION from genesis.utils.array_class for type-polymorphic parameters. The FEM solver is the historical exception: it follows the old @qd.data_oriented method pattern, and kernels added there must stay consistent with it. Never put @qd.data_oriented on non-solver classes (materials, couplers, entities).constraint_state.qacc next to constraint_state) stays keyword-only, because quadrants' positional func-argument expansion duplicates the member in the flattened call. Kernel calls from Python are unaffected.envs_idx, dofs_idx), index-valued scalars (joint_idx, geom ids, range starts/ends) - then the dynamic native Python scalars and fixed-size quadrants vectors (per-call values: positions, quaternions, penetrations, step sizes), then the tensors specific to the kernel/func (raw inputs/outputs, errno), then every state struct, then every info struct, then the static configs, then the kernel/func-specific constants-in-practice (shape integers, eps, tolerances - info by nature even when not declared as such), and finally the kernel/func-specific static compilation flags (qd.template() booleans such as is_backward). One exception: errno systematically takes the last position. The order keeps what matters most in front and sorts the rest by constant-ness (how likely each argument is to change between calls); since the constants-in-practice cluster at the tail, an anonymous constant can take its keyword without forcing keyword form onto the arguments that matter. Within the state/info/config groups the component order is fixed - dyn, rigid, collider (mpr, gjk, support_field, sdf), constraint - with leaf structs at their aggregate's slot in its declared field order, and adjoint-cache instances right after their primary. Within a group the guideline mandates nothing further (beyond the fixed component order of the struct groups): keep the existing relative order, and never reshuffle a signature more than the guideline requires. One predictable order makes every signature readable at a glance, keeps positional call sites unambiguous, and turns argument mismatches into immediate compile errors instead of silent misbindings.for loop of a kernel is auto-parallelized. Outer-scope locals mutated inside that loop are a single shared memory slot that all threads race on; the value after the loop is undefined and run-to-run unstable on GPU. The CPU backend often looks correct only because its default executor is single-threaded - do not let that fool you. To fix: serialize the loop (wrap the body in for _ in range(1):, or use qd.loop_config(serialize=True)), or use qd.atomic_min/qd.atomic_max/qd.atomic_add for pure reductions. When something "works on CPU but fails on Metal", check for this pattern before suspecting a compiler bug.i_c_ = tid; while i_c_ < n: ...) cannot be reused as a for loop variable in the same kernel; the compiler rejects it. The trailing-underscore convention for relative indices conveniently keeps these names distinct.print(...) intrinsic (there is no qd.print). Be aware that kernel print() is a silent no-op on Metal; use debug fields there instead.~/.cache/quadrants/, Numba caches), even as a diagnostic step. To bypass the quadrants offline cache for one run, set QD_OFFLINE_CACHE=0. Note that the offline cache does not pick up edits to library-side kernels, so use that variable when testing such edits.QD_PERFDISPATCH_FORCE are read at import genesis time; set them in the environment before the import, not after.jit_render.py) do not reliably pick up new closure variables. Change the input data before the call (for example, zero out n_indices to skip primitives) instead of adding new flag checks inside the JIT body.None, resolve the correct value at initialization from other options, scene contents, or runtime state (documented), and raise if the user explicitly supplies a conflicting value.IArrayType, OptionalIArrayType, FArrayType, Vec3FType, ... from genesis.typing), never a bare tuple[int, ...] or list[...]. The Options base is strict Pydantic, so a bare typed-collection field would reject the plain lists and numpy arrays users naturally pass; the aliases carry the coercion. Match how sibling option fields are typed.Sensor and nothing below it - no isinstance(x, SimpleSensor), no subclass method comparisons. When the manager needs class-specific knowledge (is some hook overridden? what buffer size is needed?), expose it as a classmethod on the base class with a conservative default, and let subclasses override it.scene.sim.coupler are not public API. Behavior a user must control belongs on entity-level options (for example, material options), not on methods of internal objects.gs.logger.warning(...), never Python's warnings.warn.except clauses are prohibited, with the single exception of exception forwarding across a multiprocessing boundary.z = z0 - 0.5*g*t^2), no ground penetration, velocity decaying to zero at rest, contact stopping a fall.main and passes with the fix.FIXME asking for physics-informed assertions later. This non-regression fallback is far better than checking nothing.gs.integrator.Euler so the finite-differenced acceleration matches the solver's, and account for rigid-body rotational inertia plus the implicit-damping first-order correction (effective_inertia = I + damping*dt) rather than loosening tolerances or distorting the geometry.enable_mujoco_compatibility solver option (off by default) makes the rigid solver reproduce MuJoCo's dynamics to floating-point tolerance, letting Genesis serve as its own baseline: toggling it on and off proves a faster or more robust replacement integrates to the same state, with no runtime dependency on MuJoCo. Tests may run both arms deliberately - flag on as ground truth, off as the shipped path - asserting parity through check_mujoco_model_consistency / check_mujoco_data_consistency in tests/utils.py. This differs from pinning a non-default flag to dodge a new default, which stays prohibited. The mode exists for maintainer-guided debugging and validation, not production, so it must match, never be fast.tests/ (rigid/, deformable/, particles/, ipc/, coupling/, sensors/, rendering/, parsers/, core/, integration/, benchmarks/) holding one file per capability (for example tests/rigid/test_collision.py, tests/sensors/test_imu.py); shared model fixtures live in the folder's conftest.py. New tests go into the existing capability file that covers them; a new file is only warranted for a genuinely new capability, never a fresh test_<one_thing>.py for a single feature.@pytest.mark.parametrize dimension on the existing test. Parametrize batched tests over n_envs=[0, 2] - multi-env is where shape bugs hide - but do not add a dead dimension that a conftest fixture already controls (such as backend).test_reject_offaxis_contact_on_authored_decomp, not test_stacking_tower_stability; the tower is how, the rejection is what. Never repeat the module or folder name as a prefix: tests/sensors/test_temperature.py::test_grid_sensor_contact_and_reset, never test_temperature_grid_....N_STEPS, POSITIONS); derived lookups stay lowercase. Read derived parameters back from the built model (get_dofs_armature(), ...) instead of duplicating literals between construction and assertions.assert_allclose / assert_equal from tests/utils.py; they handle tensors, numpy arrays, and scalars uniformly, provide extended broadcasting, and format failures with the full mismatch details. Prefer assert_equal for exact comparisons over framework-specific forms (torch.equal, np.array_equal).float(...)/.item() casts on tensor reductions. Design the scenario (for example, a warmup loop) so the assertion holds unconditionally, then assert on the full tensor with .all().envs_idx argument on getters rather than indexing the batched result: get_pos(envs_idx=[i]), not get_pos()[i].gu.quat_to_xyz(...) == 0.0, never by comparing to gu.identity_quat().assert_allclose(ref - het, OFFSET)) over reconstructing one side through tensor conversions, and vectorize per-env assertion loops into whole-batch calls.(pos[0] + off[0], pos[1] + off[1], pos[2] + off[2])), not zip-comprehensions.relative=True and relative=False.entity.get_state().pos is [B, n_verts, 3] (use [..., 2] for z); rigid entity.get_pos() is [B, 3] or [3] (use np.atleast_1d(...)[..., 2] and .all()).scene.add_entity, scene.add_sensor, scene.add_camera, gs.Scene, gs.morphs.*, gs.sensors.*, gs.options.*, gs.materials.*, gs.surfaces.* - even when there is a single option, and including calls nested inside other calls (morph=gs.morphs.Box(pos=..., size=...) on one line is a violation even if morph= has its own line). ruff will not enforce or preserve this, so it must be done by hand, and an expanded call must never be collapsed back. The one exception: a heterogeneous entity built from a list of morphs keeps each morph compact on a single line inside the list.inspect.signature rather than guessing).gravity, dt) is set explicitly even when it equals the default, so the simulated value provably matches the asserted one. This applies to plain function calls too: never spell out an argument whose value matches the default (gu.xyz_to_quat(euler, rpy=False, degrees=False) is a violation).ViewerOptions camera_pos / camera_lookat so the region of interest (the contact patch, the crossing) fills the frame; verify the framing by rendering offscreen with the same pose and looking at the image, not by guessing. A distant wide shot is useless for debugging.vis_mode="collision" on the entities of physics tests and examples; the collision geometry is what needs visual debugging with show_viewer=True.np.full, np.array, or a single-element list. Inline one-shot target vectors instead of naming them.xml.etree.ElementTree inside a fixture that returns ET.tostring(mjcf, encoding="unicode"), passed straight to the morph's file= argument (the loaders parse inline XML). Never assemble XML from string literals, textwrap.dedent, or f-string concatenation. tests/rigid/conftest.py shows the pattern.scene.viewer.stop() (or pyrender_viewer.close()) in tests or examples; viewer teardown runs automatically when the scene is destroyed, and the try/finally wrapper it usually comes with is noise.+10% or rounding up to 50 steps (so a small count lands near 50, a large one gets +10%).steps * n_envs): a 500-step, 16-env settle (8000 env-steps) is unacceptable - delete it or fit the budget. CI cost multiplies every step across the matrix.analytical_smooth_contact_pos, not fix-issue-2793. The branch outlives the issue.Speed up forward kinematics on GPU, not Remove unnecessary atomic add from root COM accumulation. The mechanism belongs in the PR description..github/pull_request_template.md), keeping each section tight and removing sections that are empty.