Skip to content

Commit

Permalink
chore: Start writing a basic style guide
Browse files Browse the repository at this point in the history
  • Loading branch information
melody-rs committed Nov 22, 2023
1 parent 2865308 commit a309848
Show file tree
Hide file tree
Showing 4 changed files with 196 additions and 1 deletion.
3 changes: 3 additions & 0 deletions .markdownlint.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"no-inline-html": false,
}
33 changes: 33 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
## How to contribute to Luminol

#### **Find a bug?**

* **Ensure the bug was not already reported** by searching on GitHub under [Issues](https://github.com/Astrabit-ST/Luminol/issues).

* If you're unable to find an open issue addressing the problem, [open a new one](https://github.com/Astrabit-ST/Luminol/issues/new). Be sure to include a **title and clear description** and a **test case** demonstrating the expected behavior that is not occurring.

* If possible, use the relevant bug report templates to create the issue.
* Try and provide a backtrace if possible.
* You can do this by piping Luminol's output into a file and causing the crash.

#### **Did you write a patch that fixes a bug?**

* Open a new GitHub pull request with the patch.

* Ensure the PR description clearly describes the problem and solution. Include the relevant issue number if applicable.

* Ensure that your fix matches the style guide. You can read it in [STYLE.md](STYLE.md).

#### **Did you fix whitespace, format code, or make a purely cosmetic patch?**

We do not generally accept these sorts of changes unless they substantially fix readability or fix a violation of the style guide.

#### **Do you intend to add a new feature or change an existing one?**

* If you are adding a feature, check that it has not been suggested by searching on GitHub under [Issues](https://github.com/Astrabit-ST/Luminol/issues). If it hasn't, please open an issue for proper discussion.

* After there is a consensus on the new feature, you are free to make a pull request.

#### **Do you have questions about the source code?**

* Ask any question about code design in the [discord server](https://discord.gg/8jZKmesKJy).
4 changes: 3 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
# Luminol

## LUMINOL IS LOOKING FOR CONTRIBUTORS! PLEASE CONTACT leelee.rs ON DISCORD OR EMAIL <[email protected]> IF YOU WANT TO HELP
## LUMINOL IS LOOKING FOR CONTRIBUTORS! PLEASE JOIN OUR DISCORD IF YOU WANT TO KNOW HOW YOU CAN HELP

![Crates.io](https://img.shields.io/crates/v/luminol)![Crates.io](https://img.shields.io/crates/l/luminol)![Crates.io](https://img.shields.io/crates/d/luminol)[![wakatime](https://wakatime.com/badge/user/5cff5352-cb55-44dc-819e-b47f231dcfa2/project/edee199a-95c3-4206-b23e-eb6f0a7e06ba.svg)](https://wakatime.com/badge/user/5cff5352-cb55-44dc-819e-b47f231dcfa2/project/edee199a-95c3-4206-b23e-eb6f0a7e06ba)![GitHub code size in bytes](https://img.shields.io/github/languages/code-size/Astrabit-ST/Luminol)[![CI](https://github.com/Astrabit-ST/Luminol/actions/workflows/rust.yml/badge.svg)](https://github.com/Astrabit-ST/Luminol/actions/workflows/rust.yml)![GitHub issues](https://img.shields.io/github/issues/Astrabit-ST/Luminol)

Luminol is an experimental remake of the RGSS RPG Maker editors in Rust with love ❤️.

Join [our discord](https://discord.gg/8jZKmesKJy) if you're interested in the project!

Luminol targets native builds with eframe. Luminol currently reads *only* rxdata (not rvdata or rvdata2, sorry VX and VX Ace users). In the past, Luminol used to exclusively read rusty object notation (ron) files made from [rmxp_extractor](https://github.com/Speak2Erase/rmxp-extractor). Now, it uses [alox-48](https://github.com/Speak2Erase/alox-48) to deserialize rxdata. It is not 100% perfect, if it does not open your project properly, [please file an issue](https://github.com/Astrabit-ST/Luminol/issues).

In the future a custom .lumina format is planned, as well as ron, rvdata 1 & 2, and json.
Expand Down
157 changes: 157 additions & 0 deletions STYLE.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,157 @@
# Luminol code style guidelines

This document defines how Luminol's codebase should be structured, formatted and written in a ruleset format.

## Table of contents

1. [Unsafe code][unsafe]
2. [Mutability][mutability]
3. [Allocations][allocations]

## Guidelines

### [1][unsafe] **Unsafe Code**

This section defines where, when, and how to write unsafe code.

- [1.1][unsafe] DO NOT WRITE UNSAFE CODE.
- [1.2][unsafe] Unsafe code is permissible if
1) A library has a poor constraint, or marks something as unsafe when it is safe
2) The rust compiler does not understand something that would be otherwise safe
3) You have no other way of doing it that would not serverely hinder code readability
- Examples:
- rodio/cpal's audio types are !Send and !Sync due to them not being thread safe on Android (we do not support Android), so it's okay to `impl Send` on types that contain it.
```rs
// src/audio.rs
pub struct Audio {
inner: Mutex<Inner>, // Use a mutex so it's at least *slightly* more thread safe.
}

struct Inner {
_output_stream: OutputStream,
output_stream_handle: OutputStreamHandle,
sinks: HashMap<Source, Sink>,
}

// We do not support android, and audio on android is the reason why OutputStream is not `Send`. This is okay.
#[allow(unsafe_code)]
unsafe impl Send for Inner {}
```
- The rust compiler does not support self referential types. Unsafe code is required to use them properly with nested refcells.
```rs
pub fn map<'a>(&'a self, id: usize) -> impl Deref<Target = rpg::Map> + DerefMut + 'a {
struct Ref<'b> {
_state: atomic_refcell::AtomicRef<'b, State>,
map_ref: dashmap::mapref::one::RefMut<'b, usize, rpg::Map>,
}
impl<'b> Deref for Ref<'b> {
type Target = rpg::Map;

fn deref(&self) -> &Self::Target {
&self.map_ref
}
}
impl<'b> DerefMut for Ref<'b> {
fn deref_mut(&mut self) -> &mut Self::Target {
&mut self.map_ref
}
}

let state = self.state.borrow();
let State::Loaded { ref maps, .. } = &*state else {
panic!("project not loaded")
};
//? # SAFETY
// For starters, this has been tested against miri. Miri is okay with it.
// Ref is self referential- map_ref borrows from _state. We need to store _state so it gets dropped at the same time as map_ref.
// If it didn't, map_ref would invalidate the refcell. We could unload the project, changing State while map_ref is live. Storing _state prevents this.
// Because the rust borrow checker isn't smart enough for this, we need to create an unbounded reference to maps to get a map out. We're not actually using this reference
// for any longer than it would be valid for (notice the fact that we assign map_ref a lifetime of 'a, which is the lifetime it should have anyway) so this is okay.
let map_ref: dashmap::mapref::one::RefMut<'a, _, _> = unsafe {
let unsafe_maps_ref: &dashmap::DashMap<usize, rpg::Map> = &*(maps as *const _);
unsafe_maps_ref
.entry(id)
.or_try_insert_with(|| {
state!()
.filesystem
.read_data(format!("Data/Map{id:0>3}.{}", self.rxdata_ext()))
})
.expect("failed to load map") // FIXME
};

Ref {
_state: state,
map_ref,
}
}
```
- Using libraries like OpenGL is naturally unsafe. If they are a necessity, then unsafe code is okay, but PLEASE validate it as much as possible first.
- [1.3][unsafe] When writing unsafe code, use as little as possible. i.e. when using `unsafe impl Send`, avoid `unsafe impl Sync` and use a Mutex.
- [1.4][unsafe] Storing raw pointers is heavily discouraged. In general, keep pointers on the stack and do not store them.
- [1.5][unsafe] Impling `Send` and `Sync` is nice as a stopgap as Luminol generally does not use multithreaded coded.
- [1.5.1][unsafe] You can use them to test things, but they are disallowed in pull requests.
- [1.5.2][unsafe] If you can prove that they are actually safe, then they are allowed (see the rodio/cpal example.)

### [2][mutability] **Mutability rules **

This section outlines when to take `&mut self`.

- [1.1][mutability] Prefer taking `&mut self`.
- [1.1.1] Allow the caller to determine how to handle mutability.
- [1.2][mutability] If it is not possible to use `&mut self` (i.e. shared ownership via an Arc is required) try and use atomics.
- [1.2.1][mutability] Use crossbeam's `Atomic<T>` type when dealing with non-primitive types that are `Copy`.
- [1.3][mutability] If you can't use `Atomic<T>`, use `AtomicRefCell<T>`.
- [1.3.1][mutability] `AtomicRefCell<T>` is preferred over `RwLock<T>`, which is preferred over `Mutex<T>`.
- [1.3.2][mutability] Always use `Mutex<T>` when a type does not impl `Sync`, rather than using `unsafe impl Sync`. See [1.3][unsafe] of unsafe code guidelines.

### [3][allocations] **Allocations**

This section covers when and when not to allocate memory.

- [1.1][allocations] Avoid allocations where possible.
- [1.1.1][allocations] The usage of `format!` and `vec!` is okay, though.
- [1.1.2][allocations] When making allocations of collections, try and predict a size (i.e. using `String::with_capacity`)
- [1.2][allocations] If you are concerned about a potential allocation hotspot, try checking that!
- [1.3][allocations] If a resource is going to be loaded into memory several times, and it is easy and sensible to cache it (i.e. textures), write a cache module.

### [4][panics] **Panics**

This section outlines when to panic and when not to panic.

- [1.1] Unwrap is okay when there's certain code condition that you generally expect to be true.
- [1.2] Explicit panics are okay when an intangible/unintended state is achieved.
```rs
pub fn map<'a>(&'a self, id: usize) -> impl Deref<Target = rpg::Map> + DerefMut + 'a {
struct Ref<'b> {
_state: atomic_refcell::AtomicRef<'b, State>,
map_ref: dashmap::mapref::one::RefMut<'b, usize, rpg::Map>,
}
// ...

// We expect to be loaded when this function is called. This function could theoretically be called anywhere, but is generally called by code that relies on the filesystem.
// When a project is not loaded, that code is never called. It is a logic error for it to be called.
let state = self.state.borrow();
let State::Loaded { ref maps, .. } = &*state else {
panic!("project not loaded")
};

// ...
Ref {
_state: state,
map_ref,
}
}
```

### [5][errors] **Errors**

## Credits

- [**speak2erase**](https://github.com/speak2erase) for writing this guide.


[unsafe]: #1-unsafe-code
[mutability]: #2-mutability-rules
[allocations]: #3-allocations
[panics]: #4-panics
[errors]: #5-errors

0 comments on commit a309848

Please sign in to comment.