Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor BspLump #23

Open
snake-biscuits opened this issue May 13, 2022 · 8 comments
Open

Refactor BspLump #23

snake-biscuits opened this issue May 13, 2022 · 8 comments
Assignees
Labels
documentation This issue involves a lack of documentation enhancement New feature or request need test We need to write a test for regression testing
Milestone

Comments

@snake-biscuits
Copy link
Owner

snake-biscuits commented May 13, 2022

Editing attrs of an object returned by a BspLump don’t update BspLump.changes, need to do some soft copies probably
Slice overriding & insertion don’t seem to work either.
Both of these need tests and more

>>> BspLump[i].x = new_val
>>> BspLump[i].x
old_val

editing a lump should be as easy as modifying the contents of a list

@snake-biscuits snake-biscuits added the bug Something isn't working label May 13, 2022
@snake-biscuits snake-biscuits added this to the v0.4.0 milestone May 13, 2022
@snake-biscuits snake-biscuits self-assigned this May 13, 2022
@snake-biscuits
Copy link
Owner Author

snake-biscuits commented May 18, 2022

Need to let copied list elements behave as if BspLump classes are real lists
Changes can be made at any time to any touched return value
Slices should return copies

__setitem__(self, index, value) should allow insertion when index is a slice
Tracking insertions with minimal changes stored would be ideal (track insertions and offset indices for __getitem__)

each BspLump type will need a test for this, do not have RespawnBsp bsp_lumps to test
Titanfall2 repo has r2 mp_lobby so use that

@snake-biscuits
Copy link
Owner Author

Changes can be made at any time to any touched return value

This wouldn't work with bytes objects, as they are immutable, but allowing changes to RawBspLump would be nice

@snake-biscuits
Copy link
Owner Author

should really address this before tackling #15 as making small edits & seeing their affects in-engine is very powerful for research
but when the bsp_tool implementation differs from user expectations you're gonna see a lot of bugs & wacky workarounds

gotta love writing my own kernel to handle big data

@snake-biscuits
Copy link
Owner Author

gotta love writing my own kernel to handle big data

No, really, we need to do garbage collection of some kind here
Each reference to an object collected will need to feed back into the changes attribute of the BspLump it came from

bsp.VERTICES[0].z += 1 # doesn't work rn. but should
{print(v) for v in bsp.VERTICES[:64]}  # makes a copy and deletes it within scope; no changes possible

Maybe we could override the __del__ method of each LumpClass instance to remove itself from changes if unchanged?

We would also need to accurately mimic how Python creates copies & deep-copies
Ideally we hook existing Python memory management systems to handle all of this for us.
Some people smarter than us may have already tackled this, idk.
Could be a bunch of talks on messing with memory management & working with large files, I haven't found them yet tho.

The odds someone smarter than me has already tried this are quite high.
Research time should be put into finding examples tackling this problem.

@snake-biscuits snake-biscuits modified the milestones: v0.5.0, v1.0.0 Mar 17, 2023
@snake-biscuits snake-biscuits changed the title BspLump editing has some bugs BspLump interface & cache Apr 17, 2023
@snake-biscuits
Copy link
Owner Author

BspLumps could use a pretty serious refactor at this point tbh.
They're not the list analogue I was hoping, & RawBspLump needs to be more like bytearray

In #80 & #68 I've planned to use BspLump to defer translating StaticPropClasses
The best way to do this would be with an alternate __init__ (@classmethod from_struct_and_count)

Generic __init__ methods w/ alternate @classmethod .from_ __init__s are nice for testing anyway

Maybe we could even rethink inheritance & external lumps enough to simplify lumps/__init__.py down to lumps.py

@snake-biscuits snake-biscuits changed the title BspLump interface & cache BspLump Refactor Apr 25, 2023
@snake-biscuits snake-biscuits changed the title BspLump Refactor Refactor BspLump Apr 25, 2023
@snake-biscuits snake-biscuits added enhancement New feature or request need test We need to write a test for regression testing labels Apr 27, 2023
@snake-biscuits
Copy link
Owner Author

Setting a read-only flag on BspLumps could be useful (checked when getting & setting)
That way anything like extensions.diff / decompile, which only care about parsing can have minimal memory impact

IMO the best way to set this would be as a kwarg when opening a .bsp

bsp_tool.load_bsp("filepath/maps/filename.bsp", read_only=True)
bsp_tool.ValveBsp(bsp_tool.branches.valve.source, "filepath/maps/filename.bsp", read_only=True)

The read_only flag could lock updates to BspLump._changes (though enforcing could be difficult)
Dealing with read_only changing dynamically seems pretty complicated...

But still, I would like to avoid creating entries in _changes when the user promises that they will only read
Otherwise, any loop called over a lump will load the entire lump into memory in it's bloated python object form

Whatever path we take, I want to give the user some control about how much RAM they use, and have good default behaviour.

@snake-biscuits
Copy link
Owner Author

Rather than worrying about potential performance impacts, test & measure different implementations

Some manual garbage collection on cached changes could be nice, idk if automation is possible

@snake-biscuits
Copy link
Owner Author

616f08d needs more tests

Don't 100% understand when _changes caches the bulk of a lump when we don't want it to
In the case of [x for x in bsp.LUMP], mutating x shouldn't update bsp.LUMP._changes

Also undecided as to whether or not slices are copies or not
Not to mention copy vs. deep copy

Behaviour should match:

  • RawBspLump -> bytearray
  • BasicBspLump -> List[int]
  • BspLump -> List[LumpClass]

@snake-biscuits snake-biscuits removed the bug Something isn't working label May 9, 2023
@snake-biscuits snake-biscuits added the documentation This issue involves a lack of documentation label Oct 22, 2023
@snake-biscuits snake-biscuits modified the milestones: v1.0.0, v0.5.0 Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation This issue involves a lack of documentation enhancement New feature or request need test We need to write a test for regression testing
Projects
Status: Todo: Test
Development

No branches or pull requests

1 participant