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

Camera API (revival) #14325

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

Conversation

Andrey2470T
Copy link
Contributor

@Andrey2470T Andrey2470T commented Jan 29, 2024

Revival of #13052. The PR also contains a few my fixes and improvements.

To do

This PR is a Work in Progress.

  • More detailed documentation for the API in lua_api.md containing an info about the default parameter values.
  • Dont render the wield hand on the secondary cameras textures.
  • Better the camera params ids reserving for new created cameras on server.
  • Remove the remain control over the main camera (should be postponed for a follow-up PR).
  • Improve attaching mechanism the cameras to the objects.
  • Occlusion culling (e.g. the portal algorithm suggested by x2048 in the original PR thread).

Probably something still...

How to test

Test cameras mod from devtest game.

@Andrey2470T Andrey2470T marked this pull request as draft January 29, 2024 22:23
@wsor4035 wsor4035 added @ Client / Audiovisuals Roadmap The change matches an item on the current roadmap. Feature ✨ PRs that add or enhance a feature @ Script API labels Jan 29, 2024
@appgurueu
Copy link
Contributor

appgurueu commented Jan 29, 2024

I think this is a pretty important feature to have, but I'm not sure "reusing" kilbith's code is fine licensing-wise (and even if it were, it might be problematic ethically). On the license side, we have headers for some files, so those should be fine. For all changes beyond that, I don't know. I don't see a reason to assume we'd be allowed to use them (one could argue try that at the point of redistributing those files, they need to have been licensed under a LGPLv2.1-compatible license to comply with copyleft, but that's a bit of a stretch).

Bottom line: Unless kilbith were to expressly license his code as LGPLv2.1 (which is unlikely), I don't think we're allowed to use the non-licensed code. I missed the obvious fact that kilbith's branch of course still contains the unabridged LGPLv2.1 license file, so it is probably fine licensing-wise. Still I'm not sure morally. If you go ahead with this, at least make sure the commit history is clean: kilbith should be first author on his commits, with x2048 as co-author (or something like that); your commits should be separate (or you could make yourself a third co-author, I suppose). But you need to take care to make sure you don't accidentally make yourself first author (which currently seems to be the case); we should at the very least give proper attribution.


Technically / conceptually, a "camera API" should be clear client Lua API ((SS)CSM) territory (this is also currently the case: the client has a camera object; the server does not - it can only tweak a few parameters such as camera offset), so I'm not sure this (server-centric) PR is the best approach.

@Andrey2470T
Copy link
Contributor Author

Andrey2470T commented Feb 1, 2024

I think this is a pretty important feature to have, but I'm not sure "reusing" kilbith's code is fine licensing-wise (and even if it were, it might be problematic ethically). On the license side, we have headers for some files, so those should be fine. For all changes beyond that, I don't know. I don't see a reason to assume we'd be allowed to use them (one could argue try that at the point of redistributing those files, they need to have been licensed under a LGPLv2.1-compatible license to comply with copyleft, but that's a bit of a stretch).

Bottom line: Unless kilbith were to expressly license his code as LGPLv2.1 (which is unlikely), I don't think we're allowed to use the non-licensed code. I missed the obvious fact that kilbith's branch of course still contains the unabridged LGPLv2.1 license file, so it is probably fine licensing-wise. Still I'm not sure morally. If you go ahead with this, at least make sure the commit history is clean: kilbith should be first author on his commits, with x2048 as co-author (or something like that); your commits should be separate (or you could make yourself a third co-author, I suppose). But you need to take care to make sure you don't accidentally make yourself first author (which currently seems to be the case); we should at the very least give proper attribution.

I merged all changes of kilbith and x2048 into the only commit since the original branch doesn't exist anymore and I assume once the PR is merged the commit history will be squashed into the one though, so it doesn't make sense to worry about the history. I also don't think the order of commit authors also has a large importance. The main thing is it is attributed to the correct people.

Technically / conceptually, a "camera API" should be clear client Lua API ((SS)CSM) territory (this is also currently the case: the client has a camera object; the server does not - it can only tweak a few parameters such as camera offset), so I'm not sure this (server-centric) PR is the best approach.

I think the API should be a bit server-oriented since there are such things as attaching textures for cameras and adding them in the node/entity defs. It is impossible to register nodes/entities and any other things client-side. Also it is necessary for attaching to objects because the objects are not accessible also through CSM except the local player itself. The task for server in this PR is saving maps of the cameras IDs and their params structures and reading them from/sending to clients. Saving camera parameters on the server is necessary e.g. to get them on the request from some mod (player:get_camera() method) or just for a check whether the camera with given ID already exists like as player:set_camera() does.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@ Client / Audiovisuals Feature ✨ PRs that add or enhance a feature Roadmap The change matches an item on the current roadmap. @ Script API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants