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

Draft: allow for more light sources #3128

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ThomasChr
Copy link
Contributor

I'm capping this to 255 for the moment because we're using it with an uint_8. Can be further increased if we change the uint_8 in lighting.h and lighting.cpp.

lighting.h, Line 43
extern uint8_t ActiveLights[MAXLIGHTS];

lighting.cpp, Line 21
uint8_t ActiveLights[MAXLIGHTS];

@ThomasChr
Copy link
Contributor Author

Draft because: Untested. See also PR #3122

@ephphatha ephphatha marked this pull request as draft October 14, 2021 10:10
@ephphatha
Copy link
Contributor

In LoadGame() (loadsave.cpp:1751) loading a non-town save file expects exactly 32 light indexes/IDs followed by a max of 32 chunks of data representing each light. This will need to have logic added to save the first 32 active lights, then put the rest somewhere else while also ensuring that ActiveLightCount is 32 in old versions but gets the correct value for versions supporting more lights.

@ephphatha
Copy link
Contributor

Draft because: Untested. See also PR #3122

When you're ready you can use the Ready for review button that appears in the workflow status area:
image

The mark as draft "button" is disguised as a text link, that appears in the side menu at the bottom of the Reviewers section, it'll be in the region I marked with a yellow box in this screenshot, but looks like a link:
image

Source/lighting.h Outdated Show resolved Hide resolved
@AJenbo
Copy link
Member

AJenbo commented Oct 14, 2021

I'm capping this to 255 for the moment because we're using it with an uint_8.

It is used with int_8, cap it to 127.

https://github.com/diasurgical/devilutionX/blob/master/Source/monster.h#L218

@ThomasChr ThomasChr force-pushed the allow_for_more_lightsources branch 3 times, most recently from a053302 to 2a2840a Compare October 19, 2021 08:49
@AJenbo
Copy link
Member

AJenbo commented Oct 22, 2021

It's a lot of noise for a single int change 😉

@ThomasChr
Copy link
Contributor Author

I'm working on the save right now. Should have a test version today...
It's a little bit more tricky then I thought.

@ThomasChr
Copy link
Contributor Author

ThomasChr commented Oct 23, 2021

Just to note here: We‘re also saving:

  1. The number of active lights - which we need to reduce by the number of berserk light.
  2. The whole active lights array. Which has MAXLIGHTS Entrys. Also here we need only save 32 id's and no berserk light ids.
  3. For every active light the details.

So there are little more tricks to do. I find it strange that we‘re saving the whole activelights array even if most of them are in fact not really active.

@ThomasChr ThomasChr force-pushed the allow_for_more_lightsources branch 16 times, most recently from b1bbc4a to 4053614 Compare October 25, 2021 10:14
@ThomasChr ThomasChr force-pushed the allow_for_more_lightsources branch 4 times, most recently from a0d46a7 to 5ebce7b Compare October 25, 2021 10:45
@ThomasChr
Copy link
Contributor Author

It was much more complicated then I thought at first.
The code does seem to work right now. I will test a little more and remove the debug messages.

@ThomasChr ThomasChr force-pushed the allow_for_more_lightsources branch 2 times, most recently from 1376707 to 50616b9 Compare October 25, 2021 11:27
@ThomasChr
Copy link
Contributor Author

crashes when saving with active firewall (missle light)...

@ThomasChr
Copy link
Contributor Author

crash solved. Needs more testing and tidying up

@ThomasChr
Copy link
Contributor Author

@AJenbo
Do we favor single line if-, or for-loops with-, or without braces?

if (true)
    doWork();

for (int i; i <= 10; i++)
    doWork(i);

or

if (true) {
    doWork();
}

for (int i; i <= 10; i++) {
    doWork(i);
}

I did find both styles

@AJenbo
Copy link
Member

AJenbo commented Oct 25, 2021

With is usually best since it's safer

@ThomasChr
Copy link
Contributor Author

Perfect. I personally prefer with braces!

@AJenbo
Copy link
Member

AJenbo commented Oct 25, 2021

Once this gets merged it would be nice if you wold work on the same for missiles :)

@AJenbo
Copy link
Member

AJenbo commented Oct 25, 2021

... you change the line endings in loadsave.cpp so now it looks like all of it is rewritten

@ThomasChr
Copy link
Contributor Author

ThomasChr commented Oct 25, 2021

Yay! Seems that I've triggered a bug... https://youtrack.jetbrains.com/issue/IDEA-262067

@ThomasChr
Copy link
Contributor Author

As noted in #3292
It should also be possible to not save lights for light objects (torches...). Because they will be applied in the game loop anyways.
So we don't save:

  • Berserk Monsters
  • Missiles
  • Light Objects

What else is there left? I know that we have light sources for unique monsters (which also can be applied after loading), and all the other light sources I can think of (player, golem...) can also be applied AFTER loading. To me there is no sense in saving lights at all. Am I wrong?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants